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

[Peixin Qiao via Phabricator via llvm-commits]

peixin added a comment.

Thanks @shraiysh for the review. Addressed the 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;
----------------
shraiysh wrote:
> 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.
These bool variables cannot declared inside the for loop. They are collected when going through all the nested direcitives and checked to emit the error messages later.


================
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{
----------------
shraiysh wrote:
> Suggestion: Considering changing this to 
> ```
> isNestedInDoSIMD = llvm::omp::doSimdSet.test(dirContext_[i].directive)
> ```
Fixed.


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


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


================
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);
----------------
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.


================
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);
----------------
shraiysh wrote:
> Nit: error message is inconsistent with others (others do not use parentheses).
Please check the commit message. These error messages are from the standard. I use the original restrictions of the standard.


================
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
----------------
shraiysh wrote:
> 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?
Added in `omp_ordered02.f90`.


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

https://reviews.llvm.org/D113399