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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

Can you mark all comments which you believe are handled as done?



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:542
+              std::string procName =
+                  parser::ToUpperCaseLetters(name->symbol->name().ToString());
+              if (procName == "MAX" || procName == "MIN" ||
----------------
yhegde wrote:
> 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 ?
Feel free to add tests. If it is from the standards document then add a comment mentioning the source.

I am thinking that everything is converted to lowercase, but I could be wrong, hence asking to check with both small and caps letters for max.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:609
+  const parser::OmpObjectList *objList{nullptr};
+  if (auto *enclosingContext{GetEnclosingDirContext()}) {
+    for (auto it : enclosingContext->clauseInfo) {
----------------
yhegde wrote:
> 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)
>            ^^^^^^^^^^^^^^
OK.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:349
+        CheckMultipleAppearances(
+            *name, *name->symbol, Symbol::Flag::OmpReduction);
+      }
----------------
yhegde wrote:
> 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 .
OK. Is there a test_symbols.sh test which checks for the symbol for non-Intrinsic reduction?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90697