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

[NimishMishra]

Summary

According to OpenMP 5.0 spec document, the following semantic restrictions have been dealt with in this patch.

1. [sections] Orphaned section directives are prohibited. That is, the section directives must appear within the sections construct and must not be encountered elsewhere in the sections region.
   
   Semantic checks for the following are not necessary, since use of orphaned section construct (i.e. without an enclosing sections directive) throws parser errors and control flow never reaches the semantic checking phase.

Added a test case for the same in llvm-project/flang/test/Parser/omp-sections01.f90

2. [sections] Must be a structured block

Added test case as llvm-project/flang/test/Semantics/omp-sections02.f90 and made changes to branching logic in llvm-project/flang/lib/Semantics/check-directive-structure.h as follows:

• NoBranchingEnforce is used within a parser::Walk called from CheckNoBranching (in check-directive-structure.h itself). CheckNoBranching is invoked from a lot of places, but of our interest in this particular context is void OmpStructureChecker::Enter(const parser::OpenMPSectionsConstruct &x) (defined within llvm-project/flang/lib/Semantics/check-omp-structure.cpp)

• [addition] NoBranchingEnforce lacked tracking context earlier (precisely once control flow entered a OpenMP directive, constructs were no longer being recorded on the ConstructStack). Relevant Pre and Post functions added for the same

• [addition] A EmitBranchOutError on encountering a CALL in a structured block

• [changes] Although NoBranchingEnforce was handling labelled EXITs and CYCLEs well, there were no semantic checks for unlabelled CYCLEs and EXITs. Support added for them as well. For unlabeled cycles and exits, there's a need to efficiently differentiate the ConstructNode added to the stack before the concerned OpenMP directive was encountered and the nodes added after the directive was encountered. The same is done through a simple private unsigned counter within NoBranchingEnforce itself

• [addition] Addition of EmitUnlabelledBranchOutError is due to lack of an error message that gave proper context to the programmer. Existing error messages are either named error messages (which aren't valid for unlabeled cycles and exits) or a simple CYCLE/EXIT statement not allowed within SECTIONS construct which is more generic than the reality.

Other than these implementations, a test case for simd construct has been added.

1. [Test case for simd] Must be a structured block / A program that branches in or out of a function with declare simd is non conforming

Uses the already existing branching out of OpenMP structured block logic as well as changes introduced in point 3 below to semantically validate the restriction. Test case added to llvm-project/flang/test/Semantics/omp-simd01.f90

---

High priority TODOs:

1. (done) Decide whether CALL is an invalid branch out of an OpenMP structured block
2. (done) Fix !$omp do's handling of unlabeled CYCLEs

[NimishMishra]

git-clang-format-7 changes

[NimishMishra]

[NimishMishra]

Fixed !$omp do's handling of unlabeled CYCLEs

[kiranchandramohan]

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?

flang/lib/Semantics/check-directive-structure.h

33

Can this be along with the other private variables in the bottom of this class?

36

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.

75

Nit: braced initialization.

flang/test/Parser/omp-sections01.f90

10–15 ↗

(On Diff #369555)

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.

flang/test/Semantics/omp-simd01.f90

0–1

This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.

25

Spelling: Invalid

[kiranchandramohan]

flang/lib/Semantics/check-directive-structure.h

35

Spelling: encountering

50

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.

66

CALL is OK in a structured block.
Remove commented code.

76–77

Nit: This comment is not required I think.

77

Why is the != check required? When is it equal to x ("DO")?
The comment below is not required.

90–92

These comments are also not required.

111

Would it be possible to make this error similar to the error in EmitBranchOutErrorWithName?

flang/test/Parser/omp-sections01.f90

1 ↗

(On Diff #369555)

This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.

flang/test/Semantics/omp-sections02.f90

2

This test is probably not working as expected. Can you change to using test_errors.py? Please consult similar existing tests in this directory.

[NimishMishra]

flang/lib/Semantics/check-directive-structure.h

50

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.

75

Replaced this with a better switch-case construct

77

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.

flang/test/Parser/omp-sections01.f90

10–15 ↗

(On Diff #369555)

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

[peixin]

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

flang/lib/Semantics/check-directive-structure.h

157

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.

flang/test/Semantics/omp-sections02.f90

22

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.

flang/test/Semantics/omp-simd01.f90

14

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?

[NimishMishra]

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

flang/lib/Semantics/check-directive-structure.h

157

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.

flang/test/Semantics/omp-sections02.f90

22

I will do this. When the discussion in other comments is resolved, will submit another diff with formatted test cases

flang/test/Semantics/omp-simd01.f90

14

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

[peixin]

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.

I understand this. What I mean is your implementation is not consistent with the summary, which should be fixed since this patch has not implement the semantic check with informative error message. Otherwise, the summary, which should be the commit message, is confusing.

flang/test/Semantics/omp-simd01.f90

14

You can replace it with print *, "sample". My key point is to check branching out to the nested loop which is not associated with simd construct. I thought out one more better test case as follows:

!$omp simd collapse(2)
do i = 1, 10
  30 print *, "This is wrong"
  do j = 1, 10
    40 print *, "This is OK"
    do k = 1, 10
      if (expression) then
        goto 30
      end if
      if (expression2) then
        goto 40
      end if
    end do
  end do
end do
!$omp end simd

If my understanding is correct, please polish this test case and add it.

[NimishMishra]

flang/test/Semantics/omp-simd01.f90

14

The constraint is there, albeit not because of my changes. When I run this case, it gives a The value of the parameter in the COLLAPSE or ORDERED clause must not be larger than the number of nested loops following the construct.

The reason for this, as it seems to me is, that when we give a COLLAPSE(2), it checks for immediately nesting of exactly 2 DO constructs. Anything else simply raises this error. In this vein, the following modification of this test case succeeds:

!$omp simd collapse(2)
do i = 1, 10  ! Removed 30 print*, ...
  do j = 1, 10
    40 print *, "This is OK"
    do k = 1, 10
      if (expression) then
        goto 30
      end if
      if (expression2) then
        goto 40
      end if
    end do
  end do
end do
!$omp end simd

Similarly, trying COLLAPSE(3) would force removal of the print statement at label 40. So I think we can be reasonably assured that the simd nesting checks implemented in check-omp-structure.cpp will assure absence of any such statements which can violate the structured block restriction.

Please let me know your thoughts on this.

[peixin]

flang/test/Semantics/omp-simd01.f90

14

Thanks for the investigation. OK to me now.

[kiranchandramohan]

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.

flang/lib/Semantics/check-directive-structure.h

41

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.

44

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.

139

Do you need the counter if you are using a private stack?

169

If only used here then you can inline the function.

flang/test/Semantics/omp-simd01.f90

0–1

Please use test_errors.py. You might need a rebase. OK to delay it till you submit it. Remove the REQUIRES shell line.

14

STOP is allowed in OpenMP structured blocks for Fortran.

[NimishMishra]

flang/lib/Semantics/check-directive-structure.h

41

Removed everything. Now I have one private vector and handle everything there only. Also removed the counter

44

Since I am no longer making a local copy, so this asset as well as the class destructor is no longer there

139

No. I have removed that now

169

Yes. Inlined the function

flang/test/Semantics/omp-sections02.f90

22

Used https://github.com/pseewald/fprettify to format all fortran source.

flang/test/Semantics/omp-simd01.f90

0–1

Shifted from using test_errors.sh. Now the test cases use test_error.py

[kiranchandramohan]

A few more questions and comments.

flang/lib/Semantics/check-directive-structure.h

19

Nit: Accidentally removed?

134–135

Wouldn't the removal of the reference result in a copy? Is that the intention here?

148

CheckForRequiredConstruct -> CheckForRequiredDoConstruct or CheckForDoConstruct

162

This function can be a lambda.

174

Looking at this again, do we need a construct stack? Can we just maintain a DoConstruct counter (which increments on entering (or Pre) a Do and decrements on leaving (or Post) a Do)? And if the DoConstruct counter value is greater than 0 then we are inside a DoConstruct, else outside.

[NimishMishra]

flang/lib/Semantics/check-directive-structure.h

19

No I don't think so. I did add #include<assert.h> when I included that assert statement. When that statement was removed, this include was also removed. I think a space may have stayed there, and got removed this time during formatting.

134–135

This is a slip from my end. I did not intend it. Will add the reference back

148

Carrying out this discussion in the other comment thread as well: on there being a counter instead of a stack

174

Yes it can be. However, note that this class NoBranchingEnforce is used by a parser::Walk within CheckNoBranching function whose aim is to check non-essential branching within OpenMP/OpenACC constructs.

Do you think, in foresight, that this private stack approach would be better than the counter approach? Just in case there may come up a use for some other construct as we expand our OpenMP/OpenACC support.

Similar thought I had for the introduced CheckForRequiredConstruct. As we expand, it could serve as the point of call from various other functions/places aiding them to make a decision on when to throw the illegal branching error.

[kiranchandramohan]

flang/lib/Semantics/check-directive-structure.h

174

In the past, there have been instances where we have replaced with a more general data-structure. If you can point out a particular case where this will be required then let us keep the construct stack and add a comment saying this will be useful when we implement that case. Otherwise, let us replace it with a counter.

Can you quickly glance through the standard and have a quick check whether there are such checks required? From the top of my head, I don't see anything but I could have missed something.

[NimishMishra]

flang/lib/Semantics/check-directive-structure.h

19

Please disregard the above comment. This was due to my editor's configuration. I will revert this space back as it was before.

162

With the counter approach, I think this won't be necessary anymore.

174

I searched through the standard. As far as I can see, I found a few other examples where this could be useful but none of them were without DoConstruct.

So I think I will remove this stack approach now and replace it with a private counter.

[kiranchandramohan]

LGTM. Thanks @NimishMishra for this patch, and addressing all the comments, and for your patience.

I have two Nits. Please have a look, fix if necessary and you can submit. You might have to trim the commit message.

flang/lib/Semantics/check-directive-structure.h

178

Nit: An underscore at the end to match others?

flang/test/Semantics/omp-simd01.f90

47

Nit: Are these ">" format characters. Can you check?

[NimishMishra]

Thanks a lot @kiranchandramohan and @peixin for suggestions on the patch.

flang/lib/Semantics/check-directive-structure.h

178

Yes will add that.

flang/test/Semantics/omp-simd01.f90

47

I used fprettify (https://github.com/pseewald/fprettify) to format the test cases. So yes this is indentation added by the same.

[josh.mottley.arm]

@NimishMishra I believe this patch is causing a false semantic error with SNAP. The error being:

error: Semantic errors in inner.f90
./inner.f90:152:7: error: CYCLE to construct outside of PARALLEL DO construct is not allowed
        IF ( g == 0 ) CYCLE

The code in snap it is erroring at in inner.f90 is:

!$OMP PARALLEL DO NUM_THREADS(nnstd_used) IF(nnstd_used>1)           &
!$OMP& SCHEDULE(STATIC,1) DEFAULT(SHARED) PRIVATE(n,g)               &
!$OMP& PROC_BIND(CLOSE)
  DO n = 1, ng_per_thrd
    g = grp_act(n,t)
    IF ( g == 0 ) CYCLE
    CALL inner_df_calc ( inno, iits(g), flux0pi(:,:,:,g),            &
                      flux0(:,:,:,g), dfmxi(g) )
  END DO
!$OMP END PARALLEL DO

AFAIK this should be valid code, and I know that @Leporacanthicus is also experiencing this issue (although it looks like there could be a secondary issue as well).
I've tried reverting this patch, and managed to stop getting this error when using the flang-omp-report plugin.

[NimishMishra]

In D108904#3096461, @josh.mottley.arm wrote:

@NimishMishra I believe this patch is causing a false semantic error with SNAP. The error being:

error: Semantic errors in inner.f90
./inner.f90:152:7: error: CYCLE to construct outside of PARALLEL DO construct is not allowed
        IF ( g == 0 ) CYCLE

The code in snap it is erroring at in inner.f90 is:

!$OMP PARALLEL DO NUM_THREADS(nnstd_used) IF(nnstd_used>1)           &
!$OMP& SCHEDULE(STATIC,1) DEFAULT(SHARED) PRIVATE(n,g)               &
!$OMP& PROC_BIND(CLOSE)
  DO n = 1, ng_per_thrd
    g = grp_act(n,t)
    IF ( g == 0 ) CYCLE
    CALL inner_df_calc ( inno, iits(g), flux0pi(:,:,:,g),            &
                      flux0(:,:,:,g), dfmxi(g) )
  END DO
!$OMP END PARALLEL DO

AFAIK this should be valid code, and I know that @Leporacanthicus is also experiencing this issue (although it looks like there could be a secondary issue as well).
I've tried reverting this patch, and managed to stop getting this error when using the flang-omp-report plugin.

@josh.mottley.arm Thanks for reporting. I triaged this. It turns out the particular class that is enforcing the semantic checks assumes the OpenMP constructs to be passed to it. In case of looping constructs however, a parser::Walk is called within OpenMP constructs rather than on them. I have provided a fix in https://github.com/llvm/llvm-project/commit/9faed889cfebf5d77faf1fab1ef8f0a2f0255e5c. Please see if this resolves your problems. I also edited a relevant test file to reflect this CYCLE condition. It wasn't there before.

[josh.mottley.arm]

@NimishMishra Thanks for the help. Looks like the patch fixes the issue I was having on my end.

flang/lib/Semantics/check-directive-structure.h

 // directive language.
 
 #ifndef FORTRAN_SEMANTICS_CHECK_DIRECTIVE_STRUCTURE_H_
 #define FORTRAN_SEMANTICS_CHECK_DIRECTIVE_STRUCTURE_H_
 
 #include "flang/Common/enum-set.h"
 #include "flang/Semantics/semantics.h"
 #include "flang/Semantics/tools.h"
+#include <assert.h>
 #include <unordered_map>
-
 namespace Fortran::semantics {
 
 template <typename C, std::size_t ClauseEnumSize> struct DirectiveClauses {
   const common::EnumSet<C, ClauseEnumSize> allowed;
   const common::EnumSet<C, ClauseEnumSize> allowedOnce;
   const common::EnumSet<C, ClauseEnumSize> allowedExclusive;
   const common::EnumSet<C, ClauseEnumSize> requiredOneOf;
 };
 
 // Generic branching checker for invalid branching out of OpenMP/OpenACC
 // directive.
 // typename D is the directive enumeration.
 template <typename D> class NoBranchingEnforce {
 public:
   NoBranchingEnforce(SemanticsContext &context,
       parser::CharBlock sourcePosition, D directive,
       std::string &&upperCaseDirName)
-      : context_{context}, sourcePosition_{sourcePosition},
+      : numPrivateConstructs{0}, context_{context},
+        sourcePosition_{sourcePosition},
         upperCaseDirName_{std::move(upperCaseDirName)}, currentDirective_{
-                                                            directive} {}
-  template <typename T> bool Pre(const T &) { return true; }
-  template <typename T> void Post(const T &) {}
+                                                            directive} {
+    privateConstructStackCopy = context_.constructStack();
+  }
+  ~NoBranchingEnforce() {
+    assert(privateConstructStackCopy == context_.constructStack());
+  }
+  template <typename T> bool Pre(const T &node) {
+    if constexpr (common::HasMember<const T *, ConstructNode>) {
+      numPrivateConstructs++;
+      context_.PushConstruct(node);
+    }
+    return true;
+  }
+  template <typename T> void Post(const T &) {
+    if constexpr (common::HasMember<const T *, ConstructNode>) {
+      numPrivateConstructs--;
+      context_.PopConstruct();
+    }
+  }
 
   template <typename T> bool Pre(const parser::Statement<T> &statement) {
     currentStatementSourcePosition_ = statement.source;
     return true;
   }
 
   void Post(const parser::ReturnStmt &) { EmitBranchOutError("RETURN"); }
   void Post(const parser::ExitStmt &exitStmt) {
     if (const auto &exitName{exitStmt.v}) {
       CheckConstructNameBranching("EXIT", exitName.value());
+    } else {
+      CheckConstructNameBranching("EXIT");
     }
   }
   void Post(const parser::StopStmt &) { EmitBranchOutError("STOP"); }
   void Post(const parser::CycleStmt &cycleStmt) {
     if (const auto &cycleName{cycleStmt.v}) {
       CheckConstructNameBranching("CYCLE", cycleName.value());
+    } else {
+      switch ((llvm::omp::Directive)currentDirective_) {
+      // exclude directives which do not need a check for unlabelled CYCLES
+      case llvm::omp::Directive::OMPD_do:
+        return;
+      case llvm::omp::Directive::OMPD_simd:
+        return;
+      default:
+        break;
+      }
+      CheckConstructNameBranching("CYCLE");
     }
   }
 
 private:
+  // tracks the number of constructs added to the ConstructStack AFTER
+  // encountering an OpenMP/OpenACC directive
+  unsigned int numPrivateConstructs;
+  ConstructStack privateConstructStackCopy;
   parser::MessageFormattedText GetEnclosingMsg() const {
     return {"Enclosing %s construct"_en_US, upperCaseDirName_};
   }
 
   void EmitBranchOutError(const char *stmt) const {
     context_
         .Say(currentStatementSourcePosition_,
             "%s statement is not allowed in a %s construct"_err_en_US, stmt,
             upperCaseDirName_)
         .Attach(sourcePosition_, GetEnclosingMsg());
   }
 
+  void EmitUnlabelledBranchOutError(const char *stmt) {
+    context_
+        .Say(currentStatementSourcePosition_,
+            "%s to construct outside of %s construct is not allowed"_err_en_US,
+            stmt, upperCaseDirName_)
+        .Attach(sourcePosition_, GetEnclosingMsg());
+  }
+
   void EmitBranchOutErrorWithName(
       const char *stmt, const parser::Name &toName) const {
     const std::string branchingToName{toName.ToString()};
     context_
         .Say(currentStatementSourcePosition_,
             "%s to construct '%s' outside of %s construct is not allowed"_err_en_US,
             stmt, branchingToName, upperCaseDirName_)
         .Attach(sourcePosition_, GetEnclosingMsg());
   }
 
   // Current semantic checker is not following OpenACC/OpenMP constructs as they
   // are not Fortran constructs. Hence the ConstructStack doesn't capture
   // OpenACC/OpenMP constructs. Apply an inverse way to figure out if a
   // construct-name is branching out of an OpenACC/OpenMP construct. The control
   // flow goes out of an OpenACC/OpenMP construct, if a construct-name from
   // statement is found in ConstructStack.
   void CheckConstructNameBranching(
       const char *stmt, const parser::Name &stmtName) {
     const ConstructStack &stack{context_.constructStack()};
+    int16_t counter = numPrivateConstructs;
     for (auto iter{stack.cend()}; iter-- != stack.cbegin();) {
       const ConstructNode &construct{*iter};
       const auto &constructName{MaybeGetNodeName(construct)};
-      if (constructName) {
+      if (counter-- < 1 && constructName) {
         if (stmtName.source == constructName->source) {
           EmitBranchOutErrorWithName(stmt, stmtName);
           return;
         }
       }
     }
   }
 
+  bool CheckForRequiredConstruct(const ConstructNode &construct) const {
+    return std::visit(common::visitors{
+                          [&](const parser::DoConstruct *) { return true; },
+                          [](const auto *) { return false; },
+                      },
+        construct);
+  }
+  // Check branching for unlabelled CYCLES and EXITs
+  void CheckConstructNameBranching(const char *stmt) {
+    // Check for associated ConstructNode within the OpenMP/OpenACC directive
+    const ConstructStack &stack{context_.constructStack()};
+    int16_t counter = numPrivateConstructs;
+    for (auto iter{stack.cend()}; iter-- != stack.cbegin();) {
+      const ConstructNode &construct{*iter};
+      // found an enclosing looping construct for the unlabelled EXIT/CYCLE
+      if (counter-- > 0 && CheckForRequiredConstruct(construct)) {
+        return;
+      }
+    }
+    // did not found an enclosing looping construct within the OpenMP/OpenACC
+    // directive
+    EmitUnlabelledBranchOutError(stmt);
+    return;
+  }
+
   SemanticsContext &context_;
   parser::CharBlock currentStatementSourcePosition_;
   parser::CharBlock sourcePosition_;
   std::string upperCaseDirName_;
   D currentDirective_;
 };
 
 // Generic structure checker for directives/clauses language such as OpenMP
 // and OpenACC.
 // typename D is the directive enumeration.
 // tyepname C is the clause enumeration.
 // typename PC is the parser class defined in parse-tree.h for the clauses.
 template <typename D, typename C, typename PC, std::size_t ClauseEnumSize>
 class DirectiveStructureChecker : public virtual BaseChecker {

flang/test/Semantics/omp-sections02.f90

+! RUN: %S/test_errors.sh %s %t %flang -fopenmp
+!REQUIRES: shell
+
+! OpenMP version 5.0.0
+! 2.8.1 sections construct
+! The code enclosed in a sections construct must be a structured block.
+program OmpConstructSections01
+      use omp_lib
+      integer :: section_count = 0
+      integer, parameter :: NT = 4 
+print *, 'section_count', section_count
+!ERROR: invalid branch into an OpenMP structured block
+!ERROR: invalid branch into an OpenMP structured block
+!ERROR: invalid branch into an OpenMP structured block
+if(NT) 20, 30, 40
+!ERROR: invalid branch into an OpenMP structured block
+goto 20
+!$omp sections
+    !$omp section
+        print*, "This is a single statement structured block"
+    !$omp section
+        open(10, file="random-file-name.txt",err=30)
+        !ERROR: invalid branch into an OpenMP structured block
+        !ERROR: invalid branch leaving an OpenMP structured block
+        open(10, file="random-file-name.txt", err=40)
+    !$omp section
+        section_count = section_count + 1
+        20 print*, 'Entering into section'
+        call calledFromWithinSection()
+        print *, 'section_count', section_count
+    !$omp section
+        section_count = section_count + 1
+        print *, 'section_count', section_count
+        !ERROR: invalid branch leaving an OpenMP structured block
+        goto 10
+    !$omp section
+        30 print *, "Error in opening file"
+!$omp end sections
+10 print*, 'Jump from section'
+
+!$omp sections
+    !$omp section
+        40 print*, 'Error in opening file'
+!$omp end sections
+end program OmpConstructSections01
+
+function returnFromSections ()
+    !$omp sections
+        !$omp section
+            !ERROR: RETURN statement is not allowed in a SECTIONS construct
+            RETURN
+    !$omp end sections
+end function
+
+subroutine calledFromWithinSection()
+    print*, "I am called from within a 'section' structured block"
+    return
+end subroutine calledFromWithinSection
+
+subroutine continueWithinSections()
+    integer i
+    do i = 1, 10
+        print*, "Statement within loop but outside section construct"
+        !$omp sections
+            !$omp section
+            IF ( i .EQ. 5  ) THEN
+                !ERROR: CYCLE to construct outside of SECTIONS construct is not allowed
+                CYCLE
+            END IF
+        !$omp end sections
+        print*, "Statement within loop but outside section contruct"
+    end do
+    
+    !$omp sections
+        !$omp section
+            do i = 1, 10
+                CYCLE
+            end do
+    !$omp end sections
+
+    !$omp sections
+        !$omp section
+            loop_1: do i = 1, 10
+                IF( i .EQ. 5 ) THEN 
+                    CYCLE loop_1
+                END IF
+            end do loop_1
+    !$omp end sections
+
+    loop_2: do i = 1, 10
+        !$omp sections
+            !$omp section
+                IF ( i .EQ. 5 ) THEN
+                    !ERROR: CYCLE to construct 'loop_2' outside of SECTIONS construct is not allowed
+                    CYCLE loop_2
+                END IF
+        !$omp end sections
+        end do loop_2
+end subroutine continueWithinSections
+
+subroutine breakWithinSections()
+    loop_3: do i = 1, 10
+        !$omp sections 
+            !$omp section
+               IF (i .EQ. 5) THEN
+                !ERROR: EXIT to construct 'loop_3' outside of SECTIONS construct is not allowed
+                    EXIT loop_3
+                END IF
+        !$omp end sections
+    end do loop_3
+
+    loop_4: do i = 1, 10
+        !$omp sections
+            !$omp section
+                IF (i .EQ. 5) THEN
+                    !ERROR: EXIT to construct outside of SECTIONS construct is not allowed
+                    EXIT
+                END IF
+        !$omp end sections
+    end do loop_4
+
+    !$omp sections
+        !$omp section
+            do i = 1, 10
+                IF(i .EQ. 5) THEN
+                    EXIT
+                END IF
+            end do
+    !$omp end sections
+
+    !$omp sections
+        !$omp section
+         loop_5: do i = 1, 10
+                IF(i .EQ. 5) THEN
+                    EXIT loop_5
+                END IF
+         end do loop_5
+    !$omp end sections
+end subroutine breakWithinSections

flang/test/Semantics/omp-sections03.f90

+! RUN: %S/test_errors.sh %s %t %flang -fopenmp
+!XFAIL: *
+! OpenMP version 5.0.0
+! 2.8.1 sections construct
+! Orphaned section directives are prohibited. That is, the section directives must appear within the sections construct and must not be encountered elsewhere in the sections region
+!TODO: Error in parsing. Make parser errors more informative. Until then, the test is XFAIL
+
+program OmpOrphanedSections
+    use omp_lib
+    integer counter
+    counter = 0
+    !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
+    !$omp section
+        print*, "An orphaned section containing a single statement"
+    !$omp section
+        counter = counter + 1
+        print*, "An orphaned section containing multiple statements"
+!$omp sections
+    !$omp section
+        print*, "Not an orphan structured block"
+!$omp end sections
+end program OmpOrphanedSections

flang/test/Semantics/omp-simd01.f90

-! RUN: not %flang -fsyntax-only -fopenmp %s 2>&1 | FileCheck %s
-! OpenMP Version 4.5
-! 2.8.1 simd Construct
-! A program that branches into or out of a simd region is non-conforming.
+! RUN: %S/test_errors.sh %s %t %flang -fopenmp
+! REQUIRES: shell
+
+! OpenMP Version 5.0
+! 2.9.3.1 simd Construct
+!   - A program that branches into or out of a simd region is non-conforming.
+!   - The associated loops must be structured blocks
 
 program omp_simd
   integer i, j
 
   !$omp simd
   do i = 1, 10
     do j = 1, 10
       print *, "omp simd"
-      !CHECK: invalid branch leaving an OpenMP structured block
+      !ERROR: invalid branch leaving an OpenMP structured block
       goto 10
     end do
+    if( i .EQ. 5 ) THEN
+         call function1()
+    else if ( i .EQ. 7 ) THEN
+        open(10, file="random-file-name.txt", err=20)
+        20 print*, "Error message doesn't branch out of the loop's structured block"
+    else 
+        !ERROR: invalid branch leaving an OpenMP structured block
+        open(10, file="random-file-name.txt", err=10)
+    end if
   end do
   !$omp end simd
-
-  !CHECK: Outside the enclosing SIMD directive
   10 stop
 
 end program omp_simd
+
+subroutine function1()
+    integer i, option
+    option = 1
+    !$omp simd
+    do i = 1, 10 
+        print*, "CORRECT SIMD LOOP"
+    end do
+    !$omp end simd
+end subroutine function1