[PATCH] D90697: [Flang][OpenMP 4.5] Add semantic check for OpenMP Reduction Clause

[Yashaswini Hegde via Phabricator via llvm-commits]

yhegde marked 3 inline comments as done.
yhegde added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:542
+              std::string procName =
+                  parser::ToUpperCaseLetters(name->symbol->name().ToString());
+              if (procName == "MAX" || procName == "MIN" ||
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > Is name->source and direct comparison with smallcase sufficient?
> > I think I was converting them to upper case because I came across a few examples with 'max' , 'min' in uppercase and few in lower case. 
> Just confirm, add a test with max and MAX.
The following example is from https://www.openmp.org//wp-content/uploads/openmp-examples-4.5.0.pdf

SUBROUTINE REDUCTION1(A, B, C, D, X, Y, N)
REAL :: X(*), A, D
INTEGER :: Y(*), N, B, C
INTEGER :: I
A = 0
B = 0
C = Y(1)
D = X(1)
!$OMP PARALLEL DO PRIVATE(I) SHARED(X, Y, N) REDUCTION(+:A) &
!$OMP& REDUCTION(IEOR:B) REDUCTION(MIN:C)  REDUCTION(MAX:D)
DO I=1,N
A = A + X(I)
B = IEOR(B, Y(I))
C = MIN(C, Y(I))
IF (D < X(I)) D = X(I)
END DO
END SUBROUTINE REDUCTION1

But probably can be ignored ?! and only lower cases sufficient ?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:609
+  const parser::OmpObjectList *objList{nullptr};
+  if (auto *enclosingContext{GetEnclosingDirContext()}) {
+    for (auto it : enclosingContext->clauseInfo) {
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > Please add a TODO here.
> > > TODO: Verify the assumption here that the immediately inclosing region is the parallel region to which the worksharing construct having reduction binds to.
> > > 
> > > Also add a check that the enclosingContext is a parallel construct.
> > > Please add a TODO here.
> > > TODO: Verify the assumption here that the immediately inclosing region is the parallel region to which the worksharing construct having reduction binds to.
> > > 
> > 
> > Can this be verified with the following cases ?
> > program red
> > !$omp sections private(k)
> >   !$omp do reduction(+:k) reduction(-:j)
> >   do i = 1, 10
> >     k = k + 1
> >   end do
> >   !$omp end do
> > !$omp end sections
> > end program red
> > 
> > program red1
> > !$omp parallel sections private(k)
> >   !$omp do reduction(+:k) reduction(-:j)
> >   do i = 1, 10
> >     k = k + 1
> >   end do
> >   !$omp end do
> > !$omp end parallel sections
> > end program red1
> > 
> > 
> > > Also add a check that the enclosingContext is a parallel construct.
> > 
> > can the following a valid testcase and hence can be added ?
> > program red
> > !$omp target private(k)
> >   !$omp sections  reduction(+:k) reduction(-:j)
> >   do i = 1, 10
> >     k = k + 1
> >   end do
> >   !$omp end sections
> > !$omp end target
> > end program red
> > 
> > 
> The ones which you specify for verifying the assumption (program red and program red1) should be caught by the nesting check (i.e. a worksharing region cannot occur in another worksharing region i.e do in sections).
> 
> ```
> program red
> !$omp sections private(k)
> !$omp do reduction(+:k) reduction(-:j)
> ```
> 
> ```
> program red1
> !$omp parallel sections private(k)
> !$omp do reduction(+:k) reduction(-:j)
> ```
> 
> The one which you specify for the enclosing context is probably a valid case and the reduction error should not apply.
> ```
> program red
> !$omp target private(k)
> !$omp sections  reduction(+:k) reduction(-:j)
> ```
> The ones which you specify for verifying the assumption (program red and program red1) should be caught by the nesting check (i.e. a worksharing region cannot occur in another worksharing region i.e do in sections).
> 
> ```
> program red
> !$omp sections private(k)
> !$omp do reduction(+:k) reduction(-:j)
> ```


yes this test case catching both the errors .

./red.f90:3:7: error: A worksharing region may not be closely nested inside a worksharing, explicit task, taskloop, critical, ordered, atomic, or master region
  !$omp do reduction(+:k) reduction(-:j)
        ^^
./red.f90:3:10: error: REDUCTION variable 'k' is PRIVATE in outer context must be shared in the parallel regions to which any of the worksharing regions arising from the worksharing construct bind.
  !$omp do reduction(+:k) reduction(-:j)
           ^^^^^^^^^^^^^^


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:349
+        CheckMultipleAppearances(
+            *name, *name->symbol, Symbol::Flag::OmpReduction);
+      }
----------------
kiranchandramohan wrote:
> yhegde wrote:
> > kiranchandramohan wrote:
> > > Where was the symbol created for these names?
> > Not sure I understood this question.  I think here symbols are created with IntrinsicOperators. 
> I saw symbols created for the reduction operation above but not for the variable.
The symbols were not created for ProcedureDesignator. So they were created here. And I think all other symbols associated with reduction are created along with IntrinsicOperators .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90697/new/

https://reviews.llvm.org/D90697