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

[Yashaswini Hegde via Phabricator via llvm-commits]

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


================
Comment at: flang/lib/Semantics/check-directive-structure.h:213
+    auto it{dirContext.clauseInfo.find(type)};
+    if (it != dirContext.clauseInfo.end()) {
+      return it->second;
----------------
kiranchandramohan wrote:
> In a multimap there can be more than one clause which maps to the same key (i.e more than one clause of the same type). Should there be an assert for only single existence?
removed multimap which was used only to reuse some part of the code.   


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


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:974
+  SymbolSourceMap outerSymbols;
+  if (auto *enclosingContext{GetEnclosingDirContext()}) {
+    if (const auto *enclosingClause{
----------------
kiranchandramohan wrote:
> There seems to be an assumption here that the immediate enclosing context is the parallel region. Is that always the case?
No such assumptions made. Included few test cases in omp-reduction07.f90 having enclosing context other than parallel construct. Hope they are ok.  


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:982
+        const auto source{it->second};
+        if (outerSymbols.find(symbol) != outerSymbols.end()) {
+          context_.Say(source,
----------------
kiranchandramohan wrote:
> Is there an assumption here that for non-sharable clauses a new symbol is created? Is that always the case? If so, can you add a comment here?
I have cleaned up this part of the code. Hope this address the review comment. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:984
+          context_.Say(source,
+              "%s variable '%s' is %s in outer context must"
+              " be shared in the parallel regions to which any"
----------------
kiranchandramohan wrote:
> Consider making this error more readable.
> 
> You can state the restriction in the standard, then add a comma and say that reduction variable x is <private/first/lastprivate> here.
Rt now separate error messages are thrown for each of these enclosing clauses . For ex:  "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."  or
"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."  Please let me know if they are not ok. 

 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90697