[PATCH] D113399: [flang][OpenMP] Add semantic checks of nesting of region about ordered construct

[Shraiysh via Phabricator via llvm-commits]

shraiysh accepted this revision.
shraiysh added a comment.
This revision is now accepted and ready to land.

Thanks for addressing the comments @peixin. One minor comment, rest LGTM. (Please wait for one more review because I am not an expert in this part of the code).



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:771-799
+    for (int i = (int)dirContext_.size() - 2; i >= 0; i--) {
+      if (llvm::omp::doSet.test(dirContext_[i].directive)) {
+        isNestedInDo = true;
+        isNestedInDoSIMD = llvm::omp::doSimdSet.test(dirContext_[i].directive);
+        if (const auto *clause{
+                FindClause(dirContext_[i], llvm::omp::Clause::OMPC_ordered)}) {
+          const auto &orderedClause{
----------------
I meant changing it this way. This will eliminate constructs like this - 
```
if(condition)
  var = true;
```
by setting `var = condition`. This change passes all the tests in the current patch. Please let me know if I have missed something here.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:784
+        } else {
+          noOrderedClause = true;
+        }
----------------
peixin wrote:
> shraiysh wrote:
> > Instead of having the `bool noOrderedClause` we can probably save the `clause` (Line #776) as a nullptr which can be used for checking later by simply checking `!clause`. (again, aiming at reducing the branches in code to make it more readable).
> `noOrderedClause` defines what it means. I think it's better to store one bool variable instead of one class object. The branches cannot be reduced even if save the `clause`.
Okay.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:794
+        context_.Say(GetContext().directiveSource,
+            "`ORDERED` region may not be closely nested inside of `CRITICAL`, "
+            "`ORDERED`, explicit `TASK` or `TASKLOOP` region."_err_en_US);
----------------
peixin wrote:
> shraiysh wrote:
> > There is no test case for this.
> This check is moved from `CheckIfDoOrderedClause` and the test case `omp-ordered-simd.f90` has been added before.
Oh okay, thanks for the clarification.


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

https://reviews.llvm.org/D113399