[PATCH] D110502: [Flang][openmp] Add semantic checks for OpenMP critical construct name resolution

[Nimish Mishra via Phabricator via llvm-commits]

NimishMishra marked 2 inline comments as done.
NimishMishra added a comment.

Thanks @kiranchandramohan and @clementval for comments on the patch. I am working on another hint clause related patch and am taking forward a little TODO to it: improving hint clause comparison.

@kiranchandramohan You mentioned cherry picking this to fir-dev. Do you want just this patch or sections patch as well in the cherry picking? Also whenever your bandwidth allows, please have a quick look at https://reviews.llvm.org/D110714. There's a little question I have posted regarded design choice of a function. Once resolved, I can spend time on that revision too and take it to closure.

Many thanks.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1055
+  if (!dirName && !ompClause.source.empty() &&
+      ompClause.source.NULTerminatedToString() != "hint(omp_sync_hint_none)") {
+    context_.Say(dir.source,
----------------
kiranchandramohan wrote:
> Nit: It would be better if you can get the value of the expression and check for it to be ZERO. You can also add a comment above indication what this check is.
> 
> 
> ```
> | | | | OmpClauseList -> OmpClause -> Hint -> Constant -> Expr = '0_4'
> | | | | | Designator -> DataRef -> Name = 'omp_sync_hint_none'
> ```
Yes this is better. 

I gave time to this, but as of now, can't figure our a clean solution to this. The problem is //OmpClause// defines a variant and that warrants its own //std::visit// or a //HasMember// thing.

This hint clause thing is still in my radar though, because a few checks still need to be done for these. So I plan to figure out a clean way (or if I can't, have another function with //std::visit// to do the comparison) and 
come back to change this.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:1058
+        parser::MessageFormattedText{
+            "Hint clause cannot exist on an unnamed CRITICAL directive"_err_en_US});
+  }
----------------
kiranchandramohan wrote:
> Nit: Consider changing to something like the following.
> 
> Hint clause with a value other than omp_sync_hint_none cannot be specified for an unnamed CRITICAL directive.
Done


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110502