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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

A few questions and change requests.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:542
+              std::string procName =
+                  parser::ToUpperCaseLetters(name->symbol->name().ToString());
+              if (procName == "MAX" || procName == "MIN" ||
----------------
Is name->source and direct comparison with smallcase sufficient?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:607
+void OmpStructureChecker::CheckMultipleAppearanceAcrossContext(
+    std::vector<const Symbol *> &currentSymbols) {
+  const parser::OmpObjectList *objList{nullptr};
----------------
Can this be const?


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


================
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()) {
----------------
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};

```


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


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:329
+        if (!name->symbol) {
+          const auto namepair{currScope().try_emplace(
+              name->source, Attrs{}, ProcEntityDetails{})};
----------------
Nit: namepair -> namePair


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:349
+        CheckMultipleAppearances(
+            *name, *name->symbol, Symbol::Flag::OmpReduction);
+      }
----------------
Where was the symbol created for these names?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90697