[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 marked 11 inline comments as done.
NimishMishra added inline comments.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:41
+                                                            directive} {
+    privateConstructStackCopy = context_.constructStack();
+  }
----------------
kiranchandramohan wrote:
> 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.
Removed everything. Now I have one private vector and handle everything there only. Also removed the counter


================
Comment at: flang/lib/Semantics/check-directive-structure.h:44
+  ~NoBranchingEnforce() {
+    assert(privateConstructStackCopy == context_.constructStack());
+  }
----------------
kiranchandramohan wrote:
> 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.
Since I am no longer making a local copy, so this asset as well as the class destructor is no longer there


================
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) {
----------------
kiranchandramohan wrote:
> Do you need the counter if you are using a private stack?
No. I have removed that now


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


================
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
----------------
NimishMishra wrote:
> 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
Used https://github.com/pseewald/fprettify to format all fortran source.


================
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:
> 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.
Shifted from using test_errors.sh. Now the test cases use test_error.py


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108904