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

[Kiran Chandramohan via Phabricator via llvm-commits]

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

Thanks @NimishMishra for this patch. I have left some comments.

Could you change the title of the review to explicitly say which semantic check is being implemented?
Can you change the description to first summarize what is being implemented, how it is implemented and then say what is left/TODO?



================
Comment at: flang/lib/Semantics/check-directive-structure.h:33
 template <typename D> class NoBranchingEnforce {
+private:
+  // tracks the number of constructs added to the ConstructStack AFTER
----------------
Can this be along with the other private variables in the bottom of this class?


================
Comment at: flang/lib/Semantics/check-directive-structure.h:35
+  // tracks the number of constructs added to the ConstructStack AFTER
+  // encoutering an OpenMP/OpenACC directive
+  uint16_t numPrivateConstructs;
----------------
Spelling: encountering


================
Comment at: flang/lib/Semantics/check-directive-structure.h:36
+  // encoutering an OpenMP/OpenACC directive
+  uint16_t numPrivateConstructs;
+
----------------
I think this can just be an unsigned integer. While we might only require a smaller integer, I don't think we use such smaller integers elsewhere.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:56
+      numPrivateConstructs--;
+      context_.PopConstruct();
+    }
----------------
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.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:66
+  // restriction (consult OpenMP 5.0)
+  // void Post(const parser::CallStmt &) { EmitBranchOutError("CALL"); }
   void Post(const parser::ReturnStmt &) { EmitBranchOutError("RETURN"); }
----------------
CALL is OK in a structured block.
Remove commented code.


================
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}) {
----------------
Nit: braced initialization.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:84-85
     if (const auto &cycleName{cycleStmt.v}) {
+      // Control flow enters here when 'CYCLE [ do-construct-name  ]' is used to
+      // cycle
       CheckConstructNameBranching("CYCLE", cycleName.value());
----------------
Nit: This comment is not required I think.


================
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
----------------
Why is the != check required? When is it equal to x ("DO")?
The comment below is not required.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:94-96
+    // Control flow reaches here is there is no need to do CYCLE checks for
+    // DoConstruct associated with OpenMPLoopConstruct as
+    // CheckCycleConstraints(...) in check-omp-structure.cpp already does that
----------------
These comments are also not required.


================
Comment at: flang/lib/Semantics/check-directive-structure.h:115
+        .Say(currentStatementSourcePosition_,
+            "invalid branch: unlabelled %s statement leaving %s construct"_err_en_US,
+            stmt, upperCaseDirName_)
----------------
Would it be possible to make this error similar to the error in EmitBranchOutErrorWithName?


================
Comment at: flang/test/Parser/omp-sections01.f90:1
+! RUN: not %flang -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s
+! OpenMP version 5.0.0
----------------
This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.


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


================
Comment at: flang/test/Semantics/omp-sections02.f90:1
+! RUN: not %flang -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s
+!REQUIRES: shell
----------------
This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.


================
Comment at: flang/test/Semantics/omp-simd01.f90:1
 ! RUN: not %flang -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s
+! OpenMP Version 5.0
----------------
This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.


================
Comment at: flang/test/Semantics/omp-simd01.f90:23
+    else 
+        !ERROR: Inavlid branch out of the OpenMP construct
+        open(10, file="random-file-name.txt", err=10)
----------------
Spelling: Invalid


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108904