[Flang][OpenMP] Parse and semantically analyze common blocks in map clauses correctly #89847
Conversation
…lauses correctly Currently, you cannot provide the common block syntax that you should be able to provide for map clauses (and that you can for declare target) e.g.: !$omp target map(tofrom: /var/) This PR seeks to change that and allow this syntax via a small tweak, which may also allow a wider range of types to be provided without issue as well via the utilisation of ResolveOmpObject a helper function used by the majority of other OmpObject handling clauses. A by product of this change, is that we now emit an error for the following syntax, when provided to map clauses with an assumed size array: !$omp target map(arr(:)) This seems inline with the specification from what I understand of it (do feel free to correct me if that is not your reading or I am incorrect!) and other OpenMP compilers i.e. gfortran, ifx, ifort.
agozillon
requested review from
skatrak,
jsjodin,
kiranchandramohan,
mjklemm and
dpalermo
llvmbot
added
flang
|
@llvm/pr-subscribers-flang-openmp Author: None (agozillon) ChangesCurrently, you cannot provide the common block syntax that you should be able to provide for map clauses (and that you can for declare target) e.g.:
This PR seeks to change that and allow this syntax via a small tweak, which may also allow a wider range of types to be provided without issue as well via the utilisation of ResolveOmpObject a helper function used by the majority of other OmpObject handling clauses. A by product of this change, is that we now emit an error for the following syntax, when provided to map clauses with an assumed size array:
This seems inline with the specification from what I understand of it (do feel free to correct me if that is not your reading or I am incorrect!) and other OpenMP compilers i.e. gfortran, ifx, ifort. Full diff: https://github.com/llvm/llvm-project/pull/89847.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 318687508ff1f5..c99b1c413970ef 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -633,6 +633,8 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
[&](const auto &name) {},
},
ompObj.u);
+
+ ResolveOmpObject(ompObj, ompFlag);
}
}
diff --git a/flang/test/Semantics/OpenMP/map-clause.f90 b/flang/test/Semantics/OpenMP/map-clause.f90
index b46b2550b04eda..a7430c3edeb949 100644
--- a/flang/test/Semantics/OpenMP/map-clause.f90
+++ b/flang/test/Semantics/OpenMP/map-clause.f90
@@ -2,9 +2,12 @@
! Check OpenMP MAP clause validity. Section 5.8.3 OpenMP 5.2.
subroutine sb(arr)
+ implicit none
real(8) :: arr(*)
real :: a
-
+ integer:: b, c, i
+ common /var/ b, c
+
!ERROR: Assumed-size whole arrays may not appear on the MAP clause
!$omp target map(arr)
do i = 1, 100
@@ -12,6 +15,7 @@ subroutine sb(arr)
enddo
!$omp end target
+ !ERROR: Assumed-size array 'arr' must have explicit final subscript upper bound value
!$omp target map(arr(:))
do i = 1, 100
a = 3.14
@@ -23,4 +27,9 @@ subroutine sb(arr)
a = 3.14
enddo
!$omp end target
+
+ !$omp target map(tofrom: /var/)
+ b = 1
+ c = 2
+ !$omp end target
end subroutine
|
|
@llvm/pr-subscribers-flang-semantics Author: None (agozillon) ChangesCurrently, you cannot provide the common block syntax that you should be able to provide for map clauses (and that you can for declare target) e.g.:
This PR seeks to change that and allow this syntax via a small tweak, which may also allow a wider range of types to be provided without issue as well via the utilisation of ResolveOmpObject a helper function used by the majority of other OmpObject handling clauses. A by product of this change, is that we now emit an error for the following syntax, when provided to map clauses with an assumed size array:
This seems inline with the specification from what I understand of it (do feel free to correct me if that is not your reading or I am incorrect!) and other OpenMP compilers i.e. gfortran, ifx, ifort. Full diff: https://github.com/llvm/llvm-project/pull/89847.diff 2 Files Affected:
diff --git a/flang/lib/Semantics/resolve-directives.cpp b/flang/lib/Semantics/resolve-directives.cpp
index 318687508ff1f5..c99b1c413970ef 100644
--- a/flang/lib/Semantics/resolve-directives.cpp
+++ b/flang/lib/Semantics/resolve-directives.cpp
@@ -633,6 +633,8 @@ class OmpAttributeVisitor : DirectiveAttributeVisitor<llvm::omp::Directive> {
[&](const auto &name) {},
},
ompObj.u);
+
+ ResolveOmpObject(ompObj, ompFlag);
}
}
diff --git a/flang/test/Semantics/OpenMP/map-clause.f90 b/flang/test/Semantics/OpenMP/map-clause.f90
index b46b2550b04eda..a7430c3edeb949 100644
--- a/flang/test/Semantics/OpenMP/map-clause.f90
+++ b/flang/test/Semantics/OpenMP/map-clause.f90
@@ -2,9 +2,12 @@
! Check OpenMP MAP clause validity. Section 5.8.3 OpenMP 5.2.
subroutine sb(arr)
+ implicit none
real(8) :: arr(*)
real :: a
-
+ integer:: b, c, i
+ common /var/ b, c
+
!ERROR: Assumed-size whole arrays may not appear on the MAP clause
!$omp target map(arr)
do i = 1, 100
@@ -12,6 +15,7 @@ subroutine sb(arr)
enddo
!$omp end target
+ !ERROR: Assumed-size array 'arr' must have explicit final subscript upper bound value
!$omp target map(arr(:))
do i = 1, 100
a = 3.14
@@ -23,4 +27,9 @@ subroutine sb(arr)
a = 3.14
enddo
!$omp end target
+
+ !$omp target map(tofrom: /var/)
+ b = 1
+ c = 2
+ !$omp end target
end subroutine
|
|
Please feel free to tag more appropriate reviewers if there are any! The suggestions were not very helpful in this case. |
| !ERROR: Assumed-size whole arrays may not appear on the MAP clause | ||
| !$omp target map(arr) | ||
| do i = 1, 100 | ||
| a = 3.14 | ||
| enddo | ||
| !$omp end target | ||
|
|
||
| !ERROR: Assumed-size array 'arr' must have explicit final subscript upper bound value |
There was a problem hiding this comment.
Annoying ping for you @mjklemm I think it's correct for us to emit an error in this case (OpenMP section syntax falls-back on fortran rules from my understanding and we also have the following OpenMP line: "The upper bound for the last dimension of an assumed-size dummy array must be specified", which I think covers this case but my Fortran and OpenMP knowledge is admittedly lacking), but just incase I thought I'd pester you for a double check!
There was a problem hiding this comment.
F2023 C930: "The second subscript shall not be omitted from a subscript-triplet in the last dimension of an assumed-size array."
There was a problem hiding this comment.
F2023 C930: "The second subscript shall not be omitted from a subscript-triplet in the last dimension of an assumed-size array."
Would it fall into any other ruling (my appologies I don't have access to a Fortran specification document in this case) as this seems to also be emitted elsewhere for regular Fortran e.g.:
subroutine s6(x)
integer :: x(*)
!ERROR: Assumed-size array 'x' must have explicit final subscript upper bound value
x(:) = [1, 2, 3]
end
And if not, does the OpenMP ruling apply in this case (e.g. "The upper bound for the last dimension of an assumed-size dummy array must be specified")?
Or is this syntax perfectly legal and should be supported by the compiler (even if other compilers seem to error out on it, although, considering they also seem to not be too friendly to the common block syntax, that might not be the best way to judge things)?
There was a problem hiding this comment.
As far as I know, the OpenMP array section syntax does not extend any Fortran array section syntax. The OpenMP spec only restricts where (clause or directive) the array section can appear. So I think the restriction,
"The second subscript shall not be omitted from a subscript-triplet in the last dimension of an assumed-size array."
is repeating C930. I will double check on it.
There was a problem hiding this comment.
Thank you very much @kkwli ! Just not wanting to accidentally increase restrictions incorrectly if it's avoidable.
There was a problem hiding this comment.
I think this is fine. But when I checked with other compilers, they don't seem to support this syntax. Did I get something wrong here?
https://fortran.godbolt.org/z/h33bzYWMr
https://fortran.godbolt.org/z/xq3czEGs8
| !ERROR: Assumed-size whole arrays may not appear on the MAP clause | ||
| !$omp target map(arr) | ||
| do i = 1, 100 | ||
| a = 3.14 | ||
| enddo | ||
| !$omp end target | ||
|
|
||
| !ERROR: Assumed-size array 'arr' must have explicit final subscript upper bound value |
In TR12, a variable list item can be "a common block name (enclosed in slashes)." |
As I think @kkwli has mentioned it seems allowed by the OpenMP specification wording, at least that's my read of it. But I will admit my reading comprehension of the specification isn't great at times (hence the likely annoying questions) |
|
Thank you both very much! I'll wait until tomorrow to land it so I can babysit the buildbots and fix any issues that might arise as it's near the end of the day here. Also gives anyone else the ability to speak up if they wish to! |
|
Going to land this just now! Thank you all for the review discussion. |
Currently, you cannot provide the common block syntax that you should be able to provide for map clauses (and that you can for declare target) e.g.:
!$omp target map(tofrom: /var/)This PR seeks to change that and allow this syntax via a small tweak, which may also allow a wider range of types to be provided without issue as well via the utilisation of ResolveOmpObject a helper function used by the majority of other OmpObject handling clauses.
A by product of this change, is that we now emit an error for the following syntax, when provided to map clauses with an assumed size array:
!$omp target map(arr(:))This seems inline with the specification from what I understand of it (do feel free to correct me if that is not your reading or I am incorrect!) and other OpenMP compilers i.e. gfortran, ifx, ifort.