[PATCH] D108904: [flang][OpenMP] Added semantic checks for sections and simd constructs

[Nimish Mishra via Phabricator via llvm-commits]

NimishMishra marked 14 inline comments as done.
NimishMishra added inline comments.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:56
+      numPrivateConstructs--;
+      context_.PopConstruct();
+    }
----------------
kiranchandramohan wrote:
> Would using a separate stack be better? Ideally, we should not modify the context stack. If in case this is better then there should be asserts to ensure that the stack is left in the same situation at the end.
I did not use the private stack because since we are already receiving the context as a SemanticsContext& through the class constructor, I thought to use to services of stack push and pop wrapper functions that class SemanticsContext offers.

Check added to ensure the context stack is in the same situation at the end as it was at the beginning.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:82
   void Post(const parser::CycleStmt &cycleStmt) {
+    std::string x = "DO";
     if (const auto &cycleName{cycleStmt.v}) {
----------------
kiranchandramohan wrote:
> Nit: braced initialization.
Replaced this with a better switch-case construct


================
Comment at: flang/lib/Semantics/check-directive-structure.h:87
       CheckConstructNameBranching("CYCLE", cycleName.value());
+    } else if (upperCaseDirName_ != x) {
+      // Control flow enters here when `CYCLE` (without any loop label) is used
----------------
kiranchandramohan wrote:
> Why is the != check required? When is it equal to x ("DO")?
> The comment below is not required.
For some directives (for example omp simd or omp do), there has to be a DO following the directive. The parser makes sure of that.

So it doesn't make sense to check unlabeled cycles for such special directives because the parser makes sure we have a matched DO that came **after** the directive was encountered in the source.

However, the string comparison was sloppy. I was replaced it with a finer switch-case with scope to add more such directives in the future.


================
Comment at: flang/test/Parser/omp-sections01.f90:10-15
+    !CHECK: expected 'END'
+    !CHECK: END PROGRAM statement
+    !CHECK: in the context: main program
+    !CHECK: expected 'END PROGRAM'
+    !CHECK: in the context: END PROGRAM statement
+    !CHECK: in the context: main program
----------------
kiranchandramohan wrote:
> Is the point here that the failure of parsing indicates that orphaned section directives are not permitted?
> I think we should have proper error messages instead of a regular parsing failure.
I had a talk with @kiranktp and he opined that the Parser is woefully sloppy with the context it gives to the error messages as well as is in itself complex to change those messages in the first place.

We discussed to return to this Parser's contextual error message issue sometime later. So for now, it's marked as a TODO and the test-case itself is XFAIL


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108904