[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))

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

As requested previously, Could you change the Summary of this PR to

-> What is implemented in this patch.
-> What is not required because it is covered by parsing.
-> What is TODO.
-> Any questions.



================
Comment at: flang/lib/Semantics/check-directive-structure.h:41
+                                                            directive} {
+    privateConstructStackCopy = context_.constructStack();
+  }
----------------
Is this copy required if we are using a privateStack? The privateStack will contain the new Constructs. I think the numPrivateConstructs variable is also not required. We can also remove the assert below.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:44
+  ~NoBranchingEnforce() {
+    assert(privateConstructStackCopy == context_.constructStack());
+  }
----------------
I don't think we use assert in the Frontend code. It could be DIE that we use. Please check.

The change above might not be required if we don't copy the construct stack.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:139
       const auto &constructName{MaybeGetNodeName(construct)};
-      if (constructName) {
+      if (counter-- < 1 && constructName) {
         if (stmtName.source == constructName->source) {
----------------
Do you need the counter if you are using a private stack?


================
Comment at: flang/lib/Semantics/check-directive-structure.h:169
+    // directive
+    EmitUnlabelledBranchOutError(stmt);
+    return;
----------------
If only used here then you can inline the function.


================
Comment at: flang/test/Semantics/omp-simd01.f90:14
   do i = 1, 10
     do j = 1, 10
       print *, "omp simd"
----------------
NimishMishra wrote:
> 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
> ```
STOP is allowed in OpenMP structured blocks for Fortran.


================
Comment at: flang/test/Semantics/omp-simd01.f90:1
 ! RUN: not %flang -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s
+! OpenMP Version 5.0
----------------
kiranchandramohan wrote:
> This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.
Please use test_errors.py. You might need a rebase. OK to delay it till you submit it.  Remove the REQUIRES shell line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108904