[PATCH] D108904: [flang][OpenMP] Added semantic checks for sections (associated section(s) should be structured block(s)) and simd constructs (associated loop(s) should be structured block(s))

[Nimish Mishra via Phabricator via llvm-commits]

NimishMishra added a comment.

In D108904#2981617 <https://reviews.llvm.org/D108904#2981617>, @peixin wrote:

> I think your implementation does deal with the first point in your summary. You only add one test case, right?

According to the spec document, I had to deal with orphaned //section// (without an enclosing //sections//). I added a test case for the same. However, presently, the parser itself is dealing correctly with the issue by giving errors with very less contextual information. Somewhere down the line, we may need to shift this check from the Parser to Semantics, or improve error reporting in the Parser itself. For now, the test case is a //XFAIL// with a //TODO// added to it.



================
Comment at: flang/lib/Semantics/check-directive-structure.h:157
+  void CheckConstructNameBranching(const char *stmt) {
+    // Check for associated ConstructNode within the OpenMP/OpenACC directive
+    const ConstructStack &stack{context_.constructStack()};
----------------
peixin wrote:
> Instead of giving these comments. Could you please give one comment such as `CheckConstructNameBranching(const char *stmt, const parser::Name &stmtName)` to differentiate their usage? Maybe this advice is not good.
I added the comments for easier review and understanding of the changes. Now since there is no problem with the approach taken, I too think I should reduce the verbosity.


================
Comment at: flang/test/Semantics/omp-sections02.f90:22
+    !$omp section
+        open(10, file="random-file-name.txt",err=30)
+        !ERROR: invalid branch into an OpenMP structured block
----------------
peixin wrote:
> Add space before `err=30`. Also please format your test cases. I think fortran test cases use two spaces on the next line instead of four spaces like the style in your part of code in `omp-simd01.f90`.
I will do this. When the discussion in other comments is resolved, will submit another diff with formatted test cases


================
Comment at: flang/test/Semantics/omp-simd01.f90:14
   do i = 1, 10
     do j = 1, 10
       print *, "omp simd"
----------------
peixin wrote:
> Does your implementation support the following code:
> 
> ```
> !$omp simd
> do i = 1, 10
>   30 stop
>   do j = 1, 10
>     goto 30
>   end do
> end do
> !$omp end simd.
> ```
> If no collapse clause specified, it behaves as if only one loop immediately following is associated with the construct. Could you please add the test in your test case?
The present state of the implementation gives a '//STOP statement is not allowed in a SIMD construct//'. Is that what you were looking for?

If not, this was added as //void Post(const parser::StopStmt &) { EmitBranchOutError("STOP"); }// in //NoBranchingEnforce// (source: https://reviews.llvm.org/D88655). Possibly @kiranchandramohan can offer some insights. According to his comment there, parallel region can have a STOP. I am not sure about SIMD.


That said, if I replace that STOP statement with something else (like the example below), no errors are generated. That is expected I believe since branching within SIMD's associated DO loop shouldn't be considered a violation of the structured block restriction.


```
 program sample                                                                                                                                                 use omp_lib                                                                                                                                                !$omp simd                                                                                                                                                   do i = 1, 10                                                                                                                                                 30 print*, "sample"                                                                                                                                          do j = 1, 10                                                                                                                                                   goto 30                                                                                                                                                      end do                                                                                                                                                     end do                                                                                                                                                     !$omp end simd                                                                                                                             end program sample
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108904