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

[Yashaswini Hegde via Phabricator via llvm-commits]

yhegde marked 2 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:
> 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. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:607
+void OmpStructureChecker::CheckMultipleAppearanceAcrossContext(
+    std::vector<const Symbol *> &currentSymbols) {
+  const parser::OmpObjectList *objList{nullptr};
----------------
kiranchandramohan wrote:
> Can this be const?
Thank you for commenting on this. I rolled back from the most of common code sharing part. 


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




================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:635
+          for (const auto *symbol : currentSymbols) {
+            if (std::find(outerSymbols.begin(), outerSymbols.end(), symbol) !=
+                outerSymbols.end()) {
----------------
kiranchandramohan wrote:
> Ideally a new symbol should be created for private clauses and reductions (see below). If new symbols are created then how will there be a match in this find? Can you explain?
> 
> ```
>   static constexpr Symbol::Flags ompFlagsRequireNewSymbol{
>       Symbol::Flag::OmpPrivate, Symbol::Flag::OmpLinear,
>       Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
>       Symbol::Flag::OmpReduction};
> 
> ```
Thats right. It was overlooked while trying to share the common code. Thank you. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:970
+
+void OmpStructureChecker::CheckCommonSymbolsInOuterCxt(
+    SymbolSourceMap &currSymbols, const llvm::omp::Clause currClause,
----------------
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




================
Comment at: flang/lib/Semantics/resolve-directives.cpp:349
+        CheckMultipleAppearances(
+            *name, *name->symbol, Symbol::Flag::OmpReduction);
+      }
----------------
kiranchandramohan wrote:
> Where was the symbol created for these names?
Not sure I understood this question.  I think here symbols are created with IntrinsicOperators. 


================
Comment at: flang/test/Semantics/omp-reduction07.f90:36
+
+  !$omp teams private(l,j),firstprivate(k)
+  !ERROR: REDUCTION variable 'k' is FIRSTPRIVATE in outer context must be shared in the parallel regions to which any of the worksharing regions arising from the worksharing construct bind.
----------------
kiranchandramohan wrote:
> From the standard: "distribute, distribute simd, distribute parallel loop, distribute parallel loop SIMD, and parallel regions, including any parallel regions arising from combined constructs, are the only OpenMP regions that may be strictly nested inside the teams region"
> 
> Nesting of an omp do worksharing loop inside a teams region is not allowed. This particular test in this file can be removed.
removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90697