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

[Shraiysh via Phabricator via llvm-commits]

shraiysh added a comment.

Hi @peixin. Thanks for the patch. I had a few comments.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:772
+    for (int i = (int)dirContext_.size() - 2; i >= 0; i--) {
+      if (llvm::omp::doSet.test(dirContext_[i].directive)) {
+        isNestedInDo = true;
----------------
There are too many branches inside the for loop. Can we make it of the form?
```
for(...) {
  isNestedInDo = ...
  isNestedInSIMD = ...
  ... <same for other bool variables> ...
  if (isNestedInDo || isNestedInSIMD || ...)
    break;
}
```
That would be more readable and will reduce the branching in this code.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:774-775
+        isNestedInDo = true;
+        if (llvm::omp::doSimdSet.test(dirContext_[i].directive))
+          isNestedInDoSIMD = true;
+        if (const auto *clause{
----------------
Suggestion: Considering changing this to 
```
isNestedInDoSIMD = llvm::omp::doSimdSet.test(dirContext_[i].directive)
```


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:780-782
+          const auto orderedValue{GetIntValue(orderedClause.v)};
+          if (orderedValue > 0)
+            isOrderedClauseWithPara = true;
----------------
Can we change this to 
```
isOrderedClauseWithPara = GetIntValue(orderedClause.v) > 0;
```


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:784
+        } else {
+          noOrderedClause = true;
+        }
----------------
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).


================
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);
----------------
There is no test case for this.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:818
+          "An ORDERED directive without the DEPEND clause must be closely "
+          "nested in a worksharing-loop (or worksharing-loop SIMD) region with "
+          "ORDERED clause without the parameter"_err_en_US);
----------------
Nit: error message is inconsistent with others (others do not use parentheses).


================
Comment at: flang/test/Semantics/omp-do06.f90:24
       print *,i
-      !ERROR: The ORDERED clause must be present on the loop construct if any ORDERED region ever binds to a loop region arising from the loop construct.
+      !ERROR: An ORDERED directive without the DEPEND clause must be closely nested in a worksharing-loop (or worksharing-loop SIMD) region with ORDERED clause without the parameter
       !$omp ordered
----------------
As far as I can tell there is no "good" testcase for this. Like
```
!$omp do ordered
do i = 1, 10
  print *, i
  !$omp ordered
      print *,i
  !$omp end ordered
end do
!$omp end do
```
Can we maybe add such testcase, if it is not already there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113399