[PATCH] D93051: [Flang][openmp] Add semantic checks for OpenMP critical construct.

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:278-282
+  const auto &directiveName{std::get<parser::Verbatim>(beginDir.t)};
+  const auto &upperCaseDirName{
+      parser::ToUpperCaseLetters(directiveName.source.ToString())};
+  const auto &beginDirName{std::get<std::optional<parser::Name>>(beginDir.t)};
+  const auto &endDirName{std::get<std::optional<parser::Name>>(endDir.t)};
----------------
Can you use parser::Unwrap<parser::Name> to simplify the code?


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:243
+// TODO: Change it to a more better version without raw string comparision
+// once the module files are present.
+static bool IsHintExprAs(
----------------
The module file has omp_sync_hint_none. 
https://github.com/llvm/llvm-project/blob/3729ee893948be7e3ba138b2a04c4cdfa6257cdf/flang/module/omp_lib.h#L46


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:260
+
+void OmpStructureChecker::Leave(const parser::OpenMPCriticalConstruct &x) {
+  // [OMP-5.0][2.17.1] Unless the effect is as if hint(omp_sync_hint_none)
----------------
Can this be part of Enter? Or is there a convention that checking clauses will only happen during leave?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:334
       Symbol::Flag::OmpFirstPrivate, Symbol::Flag::OmpLastPrivate,
-      Symbol::Flag::OmpReduction};
+      Symbol::Flag::OmpReduction, Symbol::Flag::OmpCriticalLock};
 
----------------
Does lock need a new symbol? What is the issue you saw?


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1003
+          std::get<std::optional<parser::Name>>(beginCriticalDir.t)}) {
+    ResolveOmpName(*criticalName, Symbol::Flag::OmpCriticalLock);
+  }
----------------
Can the name here be a variable declared elsewhere? If not do we have to resolve the name?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93051