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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

A few nits, comments and clarifications.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:694
+            if (const auto *symbol{name->symbol}) {
+
+              for (const auto &redOmpObject : redObjectList.v) {
----------------
Nit: remove empty line.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:700
+                    if (rsymbol->name() == symbol->name()) {
+
+                      context_.Say(GetContext().clauseSource,
----------------
Nit: remove empty line.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1064
+                      "A list item that appears in a REDUCTION clause"
+                      " should have a contiguos storage array section."_err_en_US,
+                      ContextDirectiveAsFortran());
----------------
Nit: contiguos -> contiguous


================
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:
> > 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.


================
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:
> > 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)
```


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:970
+
+void OmpStructureChecker::CheckCommonSymbolsInOuterCxt(
+    SymbolSourceMap &currSymbols, const llvm::omp::Clause currClause,
----------------
yhegde wrote:
> kiranchandramohan wrote:
> > yhegde wrote:
> > > kiranchandramohan wrote:
> > > > I guess this function should only be called if the reduction appears in a worksharing construct. Currently there seems to be no checks for that.
> > > Few more test cases added with worksharing constructs in omp-reduction07.f90.  Hope they are ok. 
> > What I meant here is that if the reduction clause does not appear in a worksharing construct (example below) then the following error does not apply.
> > 
> > ```
> > !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.
> > ```
> > 
> > ```
> > program omp_reduction
> >   
> >   integer :: i,j,l
> >   integer :: k = 10
> > 
> >   !$omp parallel private(k)
> >   !$omp simd reduction(+:k)
> >   do i = 1, 10
> >     k = k + 1
> >   end do
> >   !$omp end simd
> >   !$omp end parallel
> > 
> > end program omp_reduction
> > ```
> Thank you for explaining. I removed that case from omp-reduction07.f90. I suppose the following can be added?
> 
> 1. program red
> !$omp sections private(k)
>   !$omp sections  reduction(+:k) reduction(-:j)
>   do i = 1, 10
>     k = k + 1
>   end do
>   !$omp end sections
> !$omp end sections
> end program red
> 
> 2.  program red
> !$omp parallel sections private(k)
>   !$omp sections  reduction(+:k) reduction(-:j)
>   do i = 1, 10
>     k = k + 1
>   end do
>   !$omp end sections
> !$omp end parallel sections
> end program red
> 
> 
Sections cannot be nested inside another sections since it is a worksharing construct.


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:349
+        CheckMultipleAppearances(
+            *name, *name->symbol, Symbol::Flag::OmpReduction);
+      }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90697