[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 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;
----------------
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?


================
Comment at: flang/lib/Semantics/check-directive-structure.h:291
+template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
+void DirectiveStructureChecker<D, C, PC, ClauseEnumSize>::GetSymbolsFromClause(
+    C type, const PC *clause, SymbolSourceMap &symbols) {
----------------
Is this function OpenMP specific and hence be in the OpenMPChecker?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:512
+  const auto &ompObjectList{std::get<parser::OmpObjectList>(x.v.t)};
+  SymbolSourceMap currSymbols, enclosingSymbols;
+  GetSymbolsFromClause(
----------------
Nit: Remove enclosingSymbols  if not used.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:515
+      llvm::omp::Clause::OMPC_reduction, GetContext().clause, currSymbols);
+  CheckDefinableObjects(currSymbols, llvm::omp::Clause::OMPC_reduction);
+  CheckIntentInPointer(ompObjectList, llvm::omp::Clause::OMPC_reduction);
----------------
I guess this function can also work with ompObjectList like others. If you iterate over the names then you dont need the map from symbol to name source.


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


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:974
+  SymbolSourceMap outerSymbols;
+  if (auto *enclosingContext{GetEnclosingDirContext()}) {
+    if (const auto *enclosingClause{
----------------
There seems to be an assumption here that the immediate enclosing context is the parallel region. Is that always the case?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:982
+        const auto source{it->second};
+        if (outerSymbols.find(symbol) != outerSymbols.end()) {
+          context_.Say(source,
----------------
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?


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


================
Comment at: flang/lib/Semantics/check-omp-structure.h:203
   void CheckDependList(const parser::DataRef &);
   void CheckDependArraySection(
       const common::Indirection<parser::ArrayElement> &, const parser::Name &);
----------------
Remove this function if not used anymore.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90697