[flang][Parser] Add a node for individual sections in sections construct

[shraiysh]

Summary

This patch adds parser nodes for each indivudual section in sections
construct. This should help with the translation to FIR. !$omp section
was not recognized as a construct and hence needed special handling.

OpenMPSectionsConstruct contains a list of OpenMPConstruct. Each
such OpenMPConstruct wraps an OpenMPSectionConstruct
(section, not sections). An OpenMPSectionConstruct is a wrapper around
a Block.

[peixin]

This patch is served for lowering to MLIR for sections construct, right? You may add need one test case like https://github.com/llvm/llvm-project/blob/main/flang/test/Lower/pre-fir-tree05.f90. Testing parsing is not the target of this patch. With OpenMPConstruct in PFT, you will have evalutions for section construct.

This patch adds parser nodes for indivudual sections in sections construct.

sections -> section?

This should help with the translation to FIR.

"should" may not one good word. @NimishMishra Will this work for lowering to MLIR for sections construct?

[NimishMishra]

In D121680#3384539, @peixin wrote:

"should" may not one good word. @NimishMishra Will this work for lowering to MLIR for sections construct?

I am mentioning my understanding of these parser changes. @shraiysh please correct me if I am wrong.

Since currently sections is a std::list<Block>, during lowering we get something like the following when we try to get NestedEvaulations:

sectionsOp
         - EvaluationPartConstruct (some statement for which to generate FIR)
         - EvaluationPartConstruct
         - EvaluationPartConstruct
         - EvaluationPartConstruct

This needs special handling in the bridge wherein we refer back to the parse tree to understand which EvaluationPartConstruct belongs to which section within sections. Precisely, we try to force the following structure:

sectionsOp
   - sectionOp
         - EvaluationPartConstruct (some statement for which to generate FIR)
         - EvaluationPartConstruct
   - sectionOp
         - EvaluationPartConstruct
         - EvaluationPartConstruct

This works fine. I think through this patch, @shraiysh plans to have section (instead of EvaluationPartConstruct) when we do get the nested evaluations on sections. As far as I can see, this has the potential to remove the special handling currently being done in the bridge because it directly would give us section as nested evaluations hence removing the need to refer back to the parse tree.

[kiranktp]

LGTM. I will let others to approve this patch.

[peixin]

In D121680#3384791, @NimishMishra wrote:

In D121680#3384539, @peixin wrote:

"should" may not one good word. @NimishMishra Will this work for lowering to MLIR for sections construct?

I am mentioning my understanding of these parser changes. @shraiysh please correct me if I am wrong.

Since currently sections is a std::list<Block>, during lowering we get something like the following when we try to get NestedEvaulations:

sectionsOp
         - EvaluationPartConstruct (some statement for which to generate FIR)
         - EvaluationPartConstruct
         - EvaluationPartConstruct
         - EvaluationPartConstruct

This needs special handling in the bridge wherein we refer back to the parse tree to understand which EvaluationPartConstruct belongs to which section within sections. Precisely, we try to force the following structure:

sectionsOp
   - sectionOp
         - EvaluationPartConstruct (some statement for which to generate FIR)
         - EvaluationPartConstruct
   - sectionOp
         - EvaluationPartConstruct
         - EvaluationPartConstruct

This works fine. I think through this patch, @shraiysh plans to have section (instead of EvaluationPartConstruct) when we do get the nested evaluations on sections. As far as I can see, this has the potential to remove the special handling currently being done in the bridge because it directly would give us section as nested evaluations hence removing the need to refer back to the parse tree.

This design looks good to me.

[kiranchandramohan]

-> Can you expand on the summary/commit-message? Point out that the individual Sections will not be an OpenMP construct in the parse-tree but will be part of the OmpSectionBlock.
-> Would OpenMPSectionConstruct -> OmpSectionBlocks -> Block or OpenMPSectionConstruct -> Block be better than OmpSectionBlocks -> OpenMPSectionConstruct -> Block
-> Are there accompanying changes in the PFT?
-> Can you add a debug-dump-parse-tree test?
-> Can you check whether the flang-omp-report tool will require any changes? (flang/examples/FlangOmpReport). Our CI might be testing it.

flang/lib/Lower/OpenMP.cpp

221

Nit: sectionConstruct

flang/test/Parser/omp-sections.f90

4

The use of all caps does not seem to be in line with other tests in this directory.

[peixin]

Are there accompanying changes in the PFT?

I think there is no need. https://github.com/llvm/llvm-project/blob/3587b15abe683f164093f8d057e921f913572007/flang/include/flang/Lower/PFTBuilder.h#L136-L139 There is no need to change creating PFT if the parser node is added to parser::OpenMPConstruct. But testing -fdebug-pre-fir-tree may be necessary, just in case. If it succeeds, then there seems no need to test -fdebug-dump-parse-tree.

[shraiysh]

Rebase with main. Added parse tree and pft tests and addressed comments.

[shraiysh]

Added newline at the end of file.

[shraiysh]

@NimishMishra @peixin I have added tests showing parse tree and pft for your reference. Please let me know if there are any concerns with translation with the current structure.

In D121680#3385486, @kiranchandramohan wrote:

-> Would OpenMPSectionConstruct -> OmpSectionBlocks -> Block or OpenMPSectionConstruct -> Block be better than OmpSectionBlocks -> OpenMPSectionConstruct -> Block

While lowering, we need an OpenMPConstruct to ensure our OpenMP.cpp behaves properly with the construct. Plus we need a list of such OpenMPConstructs under OpenMPSectionsConstruct because we want genOMP to be called for each section. Also, I did not want to expose Block as one of the variants of OpenMPConstruct (having block as one of the variants might also cause some issues with the sourcing while running the flang-omp-report tool). Hence the following heirarchy:
OpenMPSectionsConstruct -> OmpSectionBlocks (list) -> OpenMPConstruct -> OpenMPSectionConstruct -> Block

-> Are there accompanying changes in the PFT?

As @peixin mentioned, PFT building doesn't need any changes. We have to add tests for it though. (I have added them now.)

-> Can you add a debug-dump-parse-tree test?

Done. Thanks for mentioning this.

-> Can you check whether the flang-omp-report tool will require any changes? (flang/examples/FlangOmpReport). Our CI might be testing it.

I have added this, but why are these checks not a part of the cmake command run by LLVM Buildbot? They should probably be added. Another thing to mention is that I am not sure if the output is accurate or not. I have added it but please let me know if there is any issue with it. (Adding @ijan1 - code owner for flang-omp-report as a reviewer).

In D121680#3388007, @peixin wrote:

But testing -fdebug-pre-fir-tree may be necessary, just in case. If it succeeds, then there seems no need to test -fdebug-dump-parse-tree.

I have added both the tests as there were no tests for sections earlier.

I have tried to address all your concerns. Apologies if I missed anything, please feel free to ping me on it.

flang/lib/Lower/OpenMP.cpp

220–223

@NimishMishra This is the addition for individual constructs.

[shraiysh]

(Adding @ijan1 - code owner for flang-omp-report as a reviewer).

I am not able to add people as reviewers. Can someone else please add @ijan1?

[peixin]

While lowering, we need an OpenMPConstruct to ensure our OpenMP.cpp behaves properly with the construct. Plus we need a list of such OpenMPConstructs under OpenMPSectionsConstruct because we want genOMP to be called for each section. Also, I did not want to expose Block as one of the variants of OpenMPConstruct (having block as one of the variants might also cause some issues with the sourcing while running the flang-omp-report tool). Hence the following heirarchy:
OpenMPSectionsConstruct -> OmpSectionBlocks (list) -> OpenMPConstruct -> OpenMPSectionConstruct -> Block

It's one top parser node, and should not be under others. It seems what you want now is that OpenMPSectionConstruct is taken as one individual construct enclosing structured-block since it should be handled separated from Sections construct when lowering. How about add both of them in OmpBlockDirective (https://github.com/llvm/llvm-project/blob/9136145eb019e1d18c966d4d06a3df349b88cc14/flang/lib/Parser/openmp-parsers.cpp#L367-L368) and delete the special parser for OpenMP sections construct? In addition, according to the standard, the code enclosed in a sections construct must be a structured block, which should be one strong reason to take OpenMPSectionsConstruct and OpenMPSectionConstruct as OpenMPBlockConstruct. But the comments should be added saying OpenMPSectionConstruct is not one real OpenMP construct, and it is for simplifying the lowering.

[peixin]

Another option is to still give OpenMPSectionsConstruct one special parser, which may be something like the following: (Each structured block in the sections construct is preceded by a section directive except possibly the first block, for which a preceding section directive is optional.)

TUPLE_CLASS_BOILERPLATE(OpenMPFirstSectionBlock)
std::tuple<std::optional<OpenMPSectionConstruct>, Block> t;

TUPLE_CLASS_BOILERPLATE(OpenMPSectionBlock)
std::tuple<OpenMPSectionConstruct, Block> t;

TUPLE_CLASS_BOILERPLATE(OpenMPSectionsConstruct)
std::tuple<OpenMPFirstSectionBlock, std::optional<OpenMPSectionBlock>> t;

Add OpenMPSectionConstruct in OpenMPConstruct in parse-tree. Also, add genFIR(const Fortran::parser::OpenMPSectionConstruct &omp) (https://github.com/llvm/llvm-project/blob/3587b15abe683f164093f8d057e921f913572007/flang/lib/Lower/Bridge.cpp#L1618)

If adding OpenMPSectionsConstruct in OpenMPBlockConstruct, one more semantic check of orphaned section directives are prohibited should be added. This method don't need the semantic check and don't need OpenMPConstruct before OpenMPSectionConstruct in parse-tree.

[shraiysh]

Hi @peixin thank you for the comments.

It's one top parser node, and should not be under others. It seems what you want now is that OpenMPSectionConstruct is taken as one individual construct enclosing structured-block since it should be handled separated from Sections construct when lowering. How about add both of them in OmpBlockDirective (https://github.com/llvm/llvm-project/blob/9136145eb019e1d18c966d4d06a3df349b88cc14/flang/lib/Parser/openmp-parsers.cpp#L367-L368) and delete the special parser for OpenMP sections construct? In addition, according to the standard, the code enclosed in a sections construct must be a structured block, which should be one strong reason to take OpenMPSectionsConstruct and OpenMPSectionConstruct as OpenMPBlockConstruct.

It cannot be an OpenMPBlockConstruct because an OpenMPBlockConstruct has an end directive associated with it but a section directive doesn't.

If adding OpenMPSectionsConstruct in OpenMPBlockConstruct, one more semantic check of orphaned section directives are prohibited should be added. This method don't need the semantic check and don't need OpenMPConstruct before OpenMPSectionConstruct in parse-tree.

Please correct me if I misunderstood this. I think you are referring to the handling of the following code

subroutine sample()
  !$omp section
    ! some code
end subroutine sample

A semantic check is not needed for this with the current structure too, as this is already a syntax error. The grammar will take care of this. I will add a testcase for this.

In D121680#3391173, @peixin wrote:

Another option is to still give OpenMPSectionsConstruct one special parser, which may be something like the following: (Each structured block in the sections construct is preceded by a section directive except possibly the first block, for which a preceding section directive is optional.)

TUPLE_CLASS_BOILERPLATE(OpenMPFirstSectionBlock)
std::tuple<std::optional<OpenMPSectionConstruct>, Block> t;

TUPLE_CLASS_BOILERPLATE(OpenMPSectionBlock)
std::tuple<OpenMPSectionConstruct, Block> t;

TUPLE_CLASS_BOILERPLATE(OpenMPSectionsConstruct)
std::tuple<OpenMPFirstSectionBlock, std::optional<OpenMPSectionBlock>> t;

Add OpenMPSectionConstruct in OpenMPConstruct in parse-tree. Also, add genFIR(const Fortran::parser::OpenMPSectionConstruct &omp) (https://github.com/llvm/llvm-project/blob/3587b15abe683f164093f8d057e921f913572007/flang/lib/Lower/Bridge.cpp#L1618)

This will also require modifications to PFTBuilder and hence isn't ideal. As you can see in genFIR(OpenMPConstruct), genFIR is called only on the nested evaluations (which are OpenMPConstruct only).

All in all, we definitely need the following hierarchy -

OpenMPConstruct -> OpenMPSectionsConstruct
  OmpSectionBlocks (list)
    OpenMPConstruct -> Block
    OpenMPConstruct -> Block
    OpenMPConstruct -> Block

I added OpenMPSectionConstruct because exposing Block to OpenMPConstruct is not ideal.

This does not need any new semantic checks for the section construct as the parser takes care of the fact that OpenMPSectionConstruct is only parsed under OpenMPSectionsConstruct.

[peixin]

It cannot be an OpenMPBlockConstruct because an OpenMPBlockConstruct has an end directive associated with it but a section directive doesn't.

I think it's ok to have one addtional end directive since it does not do anything for OpenMPSectionConstruct. Rethink about it and this does not work if there is another OpenMPConstruct inside Block of OpenMPSectionConstruct.

subroutine sample()
  !$omp section
    ! some code
end subroutine sample

A semantic check is not needed for this with the current structure too, as this is already a syntax error. The grammar will take care of this. I will add a testcase for this.

The current error is error: expected 'THREADPRIVATE' ... (a lot of code), which is not good. The error reporting info in gfortran or ifort for this case is much better. But it's the common issue in f18 semantic error reporting. So let's ignore this now. No need to add one test case for semantic check.

This will also require modifications to PFTBuilder and hence isn't ideal. As you can see in genFIR(OpenMPConstruct), genFIR is called only on the nested evaluations (which are OpenMPConstruct only).

Right. My fault. This should do not work.

flang/include/flang/Parser/parse-tree.h

3524

Can you add one comment to explain that this OpenMPConstruct is not any OpenMPConstruct. It is OpenMPConstruct->OpenMPSectionConstruct which is guaranteed by the parser type in openmp-parsers.cpp, right?

Also, one comment that you added OpenMPSectionConstruct for the first block even if there is no in source code. This is to simplify the lowering, right? How about OmpSectionBlocks be the union of block and std::list<OpenMPConstruct>, which is consistent of source code? Will it lead to much more work when lowering?

[shraiysh]

I think it's ok to have one additional end directive since it does not do anything for OpenMPSectionConstruct.

I agree, there is no issue with the additional end directive in memory. The issue is that the parser for OpenMPBlockConstruct also has a parser for OmpEndBlockDirective. This means that if we make them OpenMPBlockConstruct instead of a new OpenMPSectionConstruct, the following will be the only valid form -

!$omp sections
  !$omp section
    ! code
  !$omp end section ! not wanted
  !$omp section
    ! code
  !$omp end section
!$omp end sections

Trying to handle this for sections will require changes to all the other OpenMPBlockConstructs, which is not ideal.

Rethink about it and this does not work if there is another OpenMPConstruct inside Block of OpenMPSectionConstruct.

I am not sure I understand this, can you please elaborate (maybe with an example)? Also, by "this does not work" do you mean replacing both with OpenMPBlockConstruct does not work, or having a new OpenMPSectionConstruct does not work?

flang/include/flang/Parser/parse-tree.h

3524

Can you add one comment to explain that this OpenMPConstruct is not any OpenMPConstruct. It is OpenMPConstruct->OpenMPSectionConstruct which is guaranteed by the parser type in openmp-parsers.cpp, right?

Sure, I will do that. Thanks for pointing this out.

Yes this is to ease the lowering, however while the first construct is optional it is always there. The only reason why we would want to be accurate to input here is to avoid unnecessary semantic errors due to information added/lost while parsing. We can safely do this because there are no semantics checks for just section directive - so if it is missing in input, it will always be accurate and there will be no errors reported.

However, we can also try to do it as you mentioned. With a union of block and std::list<OpenMPConstruct> - the parser will probably get complicated. I think lowering will also be convoluted. @NimishMishra will it be non-trivial to lower this? (genOMP is only called on OpenMPConstructs).

// Implementation of this patch
OpenMPConstruct - SectionsConstruct
| - BeginSectionsDirective
| - OmpSectionBlocks
|   | - OpenMPConstruct - OpenMPSectionConstruct - Block
|   | - OpenMPConstruct - OpenMPSectionConstruct - Block
| - EndSectionsDirective

// Proposed alternate implementation
OpenMPConstruct - SectionsConstruct
| - BeginSectionsDirective
| - Block (if the first !$omp section is present, this will be empty, otherwise has to be lowered with `omp.section`)
| - OmpSectionBlocks
|   | - OpenMPConstruct - OpenMPSectionConstruct - Block
|   | - OpenMPConstruct - OpenMPSectionConstruct - Block
| - EndSectionsDirective

[NimishMishra]

flang/lib/Lower/OpenMP.cpp

220–223

Thanks @shraiysh. This is fine. With promoting section to a OpenMPConstruct, we can now use the bridge to directly lower it, without requiring any special handling.

[peixin]

I am not sure I understand this, can you please elaborate (maybe with an example)? Also, by "this does not work" do you mean replacing both with OpenMPBlockConstruct does not work, or having a new OpenMPSectionConstruct does not work?

Check the following example. What I mean is that adding OpenMPSectionConstruct as one OpenMPBlockConstruct does not work since it does not the task construct is also in its block. Of course, replacing both with OpenMPBlockConstruct does not work, either. The current method looks best to me.

subroutine sub()
  !$omp sections
      x = 111
    !$omp section
      x = 222
      !$omp task
      x = 333
      !$omp end task
  !$omp end sections
end

Please wait for @kiranchandramohan 's approve. I am not familiar with the code in FlangOmpReportVisitor.cpp.

flang/include/flang/Parser/parse-tree.h

3524

The only reason why we would want to be accurate to input here is to avoid unnecessary semantic errors due to information added/lost while parsing.

Good point. I agree. I checked OMPIRBuilder, it create one swtich case for each section code block (https://github.com/llvm/llvm-project/blob/42e8e00189be787b4d916c6d297a8315998c7687/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L1118). Also clang generate the switch case in LLVMIR even there is no section construct.

void sub() {
  int x;
  #pragma omp sections
  {
    x = 111;
  }
}

omp.inner.for.body:                               ; preds = %omp.inner.for.cond
  %7 = load i32, i32* %.omp.sections.iv., align 4
  switch i32 %7, label %.omp.sections.exit [
    i32 0, label %.omp.sections.case
  ]

So, I think there is no need to use the union of block and std::list<OpenMPConstruct>. Some comments are enough.

[kiranchandramohan]

In D121680#3389127, @shraiysh wrote:

@NimishMishra @peixin I have added tests showing parse tree and pft for your reference. Please let me know if there are any concerns with translation with the current structure.

In D121680#3385486, @kiranchandramohan wrote:

-> Would OpenMPSectionConstruct -> OmpSectionBlocks -> Block or OpenMPSectionConstruct -> Block be better than OmpSectionBlocks -> OpenMPSectionConstruct -> Block

While lowering, we need an OpenMPConstruct to ensure our OpenMP.cpp behaves properly with the construct. Plus we need a list of such OpenMPConstructs under OpenMPSectionsConstruct because we want genOMP to be called for each section. Also, I did not want to expose Block as one of the variants of OpenMPConstruct (having block as one of the variants might also cause some issues with the sourcing while running the flang-omp-report tool). Hence the following heirarchy:
OpenMPSectionsConstruct -> OmpSectionBlocks (list) -> OpenMPConstruct -> OpenMPSectionConstruct -> Block

-> Are there accompanying changes in the PFT?

As @peixin mentioned, PFT building doesn't need any changes. We have to add tests for it though. (I have added them now.)

I think it was required with the previous revision where the OpenMPSection directive was not parsed as an OpenMPConstruct.
Technically speaking, the parse-tree is a mostly faithful representation of the source. OpenMPSection is a directive according to the standard and not a Construct, at least it should not be parsed in as a construct. I was pointing to the flang-omp-report because there might be tools/plugin depending on the parse-tree to report the OpenMP constructs and we would need to add changes there as well.

I was thinking that you would be parsing it inside a section block but adjusting it later on in the PFT as an OpenMPConstruct for ease of lowering.

[shraiysh]

OpenMPSection is a directive according to the standard and not a Construct

Actually OpenMPSection is kind of in a gray area, and intuitively, I agreed with your statement. Luckily, if you check out the latest spec (5.0 or 5.1) it is sometimes mentioned as section construct and the other times mentioned as section directive. Acc to the definitions of construct and directives, section fits in the construct definition. So, section must be a construct, and in that case, this patch works out fine. Please let me know if I am missing something here.

[kiranchandramohan]

In D121680#3391890, @shraiysh wrote:

OpenMPSection is a directive according to the standard and not a Construct

Actually OpenMPSection is kind of in a gray area, and intuitively, I agreed with your statement. Luckily, if you check out the latest spec (5.0 or 5.1) it is sometimes mentioned as section construct and the other times mentioned as section directive. Acc to the definitions of construct and directives, section fits in the construct definition. So, section must be a construct, and in that case, this patch works out fine. Please let me know if I am missing something here.

Ahh, I missed that since in the OpenMP sections entry, section is always described as a directive. LGTM.

[shraiysh]

Added comment. Thanks for the review @kiranchandramohan @peixin @NimishMishra.

flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp

return std::visit(
                 [&](const auto &o) -> SourcePosition {
                   const CharBlock &source{std::get<Verbatim>(o.t).source};
                   return parsing->allCooked()
                       .GetSourcePositionRange(source)
                       ->first;
                 },
                 c.u);
           },
+          [&](const OpenMPSectionConstruct &c) -> SourcePosition {
+            const CharBlock &source{c.source};
+            return (parsing->allCooked().GetSourcePositionRange(source))->first;
+          },
       },
       c.u);
 }
 
 std::string OpenMPCounterVisitor::getName(const OmpWrapperType &w) {
   if (auto *val = std::get_if<const OpenMPConstruct *>(&w)) {
     const OpenMPConstruct *o{*val};
     return getName(*o);
return std::visit(
                 [&](const auto &c) {
                   // Get source from the verbatim fields
                   const CharBlock &source{std::get<Verbatim>(c.t).source};
                   return "atomic-" +
                       normalize_construct_name(source.ToString());
                 },
                 c.u);
           },
+          [&](const OpenMPSectionConstruct &c) -> std::string {
+            return "section";
+          },
           // OpenMPSectionsConstruct, OpenMPLoopConstruct,
           // OpenMPBlockConstruct, OpenMPCriticalConstruct Get the source from
           // the directive field of the begin directive or from the verbatim
           // field of the begin directive in Critical
           [&](const auto &c) -> std::string {
             const CharBlock &source{std::get<0>(std::get<0>(c.t).t).source};
             return normalize_construct_name(source.ToString());
           },

flang/include/flang/Parser/dump-parse-tree.h

#include "llvm/Frontend/OpenMP/OMP.inc"
   NODE(parser, OmpMemoryOrderClause)
   NODE(parser, OmpAtomicClause)
   NODE(parser, OmpAtomicClauseList)
   NODE(parser, OpenMPFlushConstruct)
   NODE(parser, OpenMPLoopConstruct)
   NODE(parser, OpenMPExecutableAllocate)
   NODE(parser, OpenMPSimpleStandaloneConstruct)
   NODE(parser, OpenMPStandaloneConstruct)
+  NODE(parser, OpenMPSectionConstruct)
   NODE(parser, OpenMPSectionsConstruct)
   NODE(parser, OpenMPThreadprivate)
   NODE(parser, OpenStmt)
   NODE(parser, Optional)
   NODE(parser, OptionalStmt)
   NODE(parser, OtherSpecificationStmt)
   NODE(parser, OutputImpliedDo)
   NODE(parser, OutputItem)

flang/include/flang/Parser/parse-tree.h

struct OmpEndSectionsDirective {
   CharBlock source;
 };
 
 // [!$omp section]
 //    structured-block
 // [!$omp section
 //    structured-block]
 // ...
-WRAPPER_CLASS(OmpSectionBlocks, std::list<Block>);
+struct OpenMPSectionConstruct {
+  WRAPPER_CLASS_BOILERPLATE(OpenMPSectionConstruct, Block);
+  CharBlock source;
+};
+
+// WRAPPER_CLASS(OpenMPSectionConstruct, Block);
+WRAPPER_CLASS(OmpSectionBlocks, std::list<OpenMPConstruct>);
 
 struct OpenMPSectionsConstruct {
   TUPLE_CLASS_BOILERPLATE(OpenMPSectionsConstruct);
   std::tuple<OmpBeginSectionsDirective, OmpSectionBlocks,
       OmpEndSectionsDirective>
       t;
 };
 
struct OpenMPLoopConstruct {
   std::tuple<OmpBeginLoopDirective, std::optional<DoConstruct>,
       std::optional<OmpEndLoopDirective>>
       t;
 };
 
 struct OpenMPConstruct {
   UNION_CLASS_BOILERPLATE(OpenMPConstruct);
   std::variant<OpenMPStandaloneConstruct, OpenMPSectionsConstruct,
-      OpenMPLoopConstruct, OpenMPBlockConstruct, OpenMPAtomicConstruct,
-      OpenMPDeclarativeAllocate, OpenMPExecutableAllocate,
-      OpenMPCriticalConstruct>
+      OpenMPSectionConstruct, OpenMPLoopConstruct, OpenMPBlockConstruct,
+      OpenMPAtomicConstruct, OpenMPDeclarativeAllocate,
+      OpenMPExecutableAllocate, OpenMPCriticalConstruct>
       u;
 };
 
 // Parse tree nodes for OpenACC 3.1 directives and clauses
 
 struct AccObject {
   UNION_CLASS_BOILERPLATE(AccObject);
   std::variant<Designator, /*common block*/ Name> u;

flang/lib/Lower/OpenMP.cpp

std::visit(
           [&](const Fortran::parser::OpenMPStandaloneConstruct
                   &standaloneConstruct) {
             genOMP(converter, eval, standaloneConstruct);
           },
           [&](const Fortran::parser::OpenMPSectionsConstruct
                   &sectionsConstruct) {
             TODO(converter.getCurrentLocation(), "OpenMPSectionsConstruct");
           },
+          [&](const Fortran::parser::OpenMPSectionConstruct &sectionConstruct) {
+            TODO(converter.getCurrentLocation(), "OpenMPSectionConstruct");
+          },
           [&](const Fortran::parser::OpenMPLoopConstruct &loopConstruct) {
             TODO(converter.getCurrentLocation(), "OpenMPLoopConstruct");
           },
           [&](const Fortran::parser::OpenMPDeclarativeAllocate
                   &execAllocConstruct) {
             TODO(converter.getCurrentLocation(), "OpenMPDeclarativeAllocate");
           },
           [&](const Fortran::parser::OpenMPExecutableAllocate
                   &execAllocConstruct) {

flang/lib/Parser/openmp-parsers.cpp

 TYPE_PARSER(sourced(construct<OmpBeginSectionsDirective>(
     sourced(Parser<OmpSectionsDirective>{}), Parser<OmpClauseList>{})))
 TYPE_PARSER(
     startOmpLine >> sourced(construct<OmpEndSectionsDirective>(
                         sourced("END"_tok >> Parser<OmpSectionsDirective>{}),
                         Parser<OmpClauseList>{})))
 
 // OMP SECTION-BLOCK
+
+TYPE_PARSER(construct<OpenMPSectionConstruct>(block))
+
 TYPE_PARSER(maybe(startOmpLine >> "SECTION"_tok / endOmpLine) >>
-    construct<OmpSectionBlocks>(
-        nonemptySeparated(block, startOmpLine >> "SECTION"_tok / endOmpLine)))
+    construct<OmpSectionBlocks>(nonemptySeparated(
+        construct<OpenMPConstruct>(sourced(Parser<OpenMPSectionConstruct>{})),
+        startOmpLine >> "SECTION"_tok / endOmpLine)))
 
-// OMP SECTIONS (2.7.2), PARALLEL SECTIONS (2.11.2)
+// OMP SECTIONS (OpenMP 5.0 - 2.8.1), PARALLEL SECTIONS (OpenMP 5.0 - 2.13.3)
 TYPE_PARSER(construct<OpenMPSectionsConstruct>(
     Parser<OmpBeginSectionsDirective>{} / endOmpLine,
     Parser<OmpSectionBlocks>{}, Parser<OmpEndSectionsDirective>{} / endOmpLine))
 
 TYPE_CONTEXT_PARSER("OpenMP construct"_en_US,
     startOmpLine >>
         first(construct<OpenMPConstruct>(Parser<OpenMPSectionsConstruct>{}),
             construct<OpenMPConstruct>(Parser<OpenMPLoopConstruct>{}),

flang/lib/Parser/unparse.cpp

void Unparse(const OmpSectionsDirective &x) {
     }
   }
   void Unparse(const OmpSectionBlocks &x) {
     for (const auto &y : x.v) {
       BeginOpenMP();
       Word("!$OMP SECTION");
       Put("\n");
       EndOpenMP();
-      Walk(y, ""); // y is Block
+      // y.u is an OpenMPSectionConstruct
+      // (y.u).v is Block
+      Walk(std::get<OpenMPSectionConstruct>(y.u).v, "");
     }
   }
   void Unparse(const OpenMPSectionsConstruct &x) {
     BeginOpenMP();
     Word("!$OMP ");
     Walk(std::get<OmpBeginSectionsDirective>(x.t));
     Put("\n");
     EndOpenMP();

flang/lib/Semantics/check-omp-structure.cpp

void OmpStructureChecker::Enter(const parser::OpenMPSectionsConstruct &x) {
   const auto &endSectionsDir{std::get<parser::OmpEndSectionsDirective>(x.t)};
   const auto &beginDir{
       std::get<parser::OmpSectionsDirective>(beginSectionsDir.t)};
   const auto &endDir{std::get<parser::OmpSectionsDirective>(endSectionsDir.t)};
   CheckMatching<parser::OmpSectionsDirective>(beginDir, endDir);
 
   PushContextAndClauseSets(beginDir.source, beginDir.v);
   const auto &sectionBlocks{std::get<parser::OmpSectionBlocks>(x.t)};
-  for (const auto &block : sectionBlocks.v) {
-    CheckNoBranching(block, beginDir.v, beginDir.source);
+  for (const parser::OpenMPConstruct &block : sectionBlocks.v) {
+    CheckNoBranching(std::get<parser::OpenMPSectionConstruct>(block.u).v,
+        beginDir.v, beginDir.source);
   }
   HasInvalidWorksharingNesting(
       beginDir.source, llvm::omp::nestedWorkshareErrSet);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPSectionsConstruct &) {
   dirContext_.pop_back();
 }

flang/test/Examples/omp-sections.f90

+! REQUIRES: plugins, examples, shell
+
+! RUN: %flang_fc1 -load %llvmshlibdir/flangOmpReport.so -plugin flang-omp-report -fopenmp %s -o - | FileCheck %s
+
+subroutine omp_sections()
+  integer :: x
+  !$omp sections private(x)
+    !$omp section
+    call f1()
+    !$omp section
+    call f2()
+  !$omp end sections nowait
+end subroutine omp_sections
+
+!CHECK: - file:            {{.*}}
+!CHECK:   line:            9
+!CHECK:   construct:       section
+!CHECK:   clauses:         []
+!CHECK: - file:            {{.*}}
+!CHECK:   line:            11
+!CHECK:   construct:       section
+!CHECK:   clauses:         []
+!CHECK: - file:            {{.*}}
+!CHECK:   line:            7
+!CHECK:   construct:       sections
+!CHECK:   clauses:
+!CHECK:     - clause:          nowait
+!CHECK:       details:         ''
+!CHECK:     - clause:          private
+!CHECK:       details:         x

flang/test/Lower/OpenMP/sections-pft.f90

+! RUN: %flang_fc1 -fdebug-pre-fir-tree -fopenmp %s | FileCheck %s
+
+subroutine openmp_sections(x, y)
+
+  integer, intent(inout)::x, y
+
+!==============================================================================
+! empty construct
+!==============================================================================
+!$omp sections
+!$omp end sections
+
+!CHECK: OpenMPConstruct
+!CHECK: End OpenMPConstruct
+
+!==============================================================================
+! single section, without `!$omp section`
+!==============================================================================
+!$omp sections
+    call F1()
+!$omp end sections
+
+!CHECK: OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK: End OpenMPConstruct
+
+!==============================================================================
+! single section with `!$omp section`
+!==============================================================================
+!$omp sections
+  !$omp section
+    call F1
+!$omp end sections
+
+!CHECK: OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK: End OpenMPConstruct
+
+!==============================================================================
+! multiple sections
+!==============================================================================
+!$omp sections
+  !$omp section
+    call F1
+  !$omp section
+    call F2
+  !$omp section
+    call F3
+!$omp end sections
+
+!CHECK: OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK: End OpenMPConstruct
+
+!==============================================================================
+! multiple sections with clauses
+!==============================================================================
+!$omp sections PRIVATE(x) FIRSTPRIVATE(y)
+  !$omp section
+    call F1
+  !$omp section
+    call F2
+  !$omp section
+    call F3
+!$omp end sections NOWAIT
+
+!CHECK: OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK:  OpenMPConstruct
+!CHECK:   CallStmt
+!CHECK:  End OpenMPConstruct
+!CHECK: End OpenMPConstruct
+
+end subroutine openmp_sections

flang/test/Parser/omp-sections.f90

+! RUN: %flang_fc1 -fdebug-unparse -fopenmp %s | FileCheck --ignore-case %s
+! RUN: %flang_fc1 -fdebug-dump-parse-tree -fopenmp %s | FileCheck --check-prefix="PARSE-TREE" %s
+
+subroutine openmp_sections(x, y)
+
+  integer, intent(inout)::x, y
+
+!==============================================================================
+! empty construct
+!==============================================================================
+!CHECK: !$omp sections
+!$omp sections
+  !CHECK: !$omp section
+!CHECK: !$omp end sections
+!$omp end sections
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPSectionsConstruct
+!PARSE-TREE: OmpBeginSectionsDirective
+!PARSE-TREE-NOT: ExecutionPartConstruct
+!PARSE-TREE: OmpEndSectionsDirective
+
+!==============================================================================
+! single section, without `!$omp section`
+!==============================================================================
+!CHECK: !$omp sections
+!$omp sections
+  !CHECK: !$omp section
+    !CHECK: CALL
+    call F1()
+!CHECK: !$omp end sections
+!$omp end sections
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPSectionsConstruct
+!PARSE-TREE:  OmpBeginSectionsDirective
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE-NOT: ExecutionPartConstruct
+!PARSE-TREE:  OmpEndSectionsDirective
+
+!==============================================================================
+! single section with `!$omp section`
+!==============================================================================
+!CHECK: !$omp sections
+!$omp sections
+  !CHECK: !$omp section
+  !$omp section
+    !CHECK: CALL F1
+    call F1
+!CHECK: !$omp end sections
+!$omp end sections
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPSectionsConstruct
+!PARSE-TREE:  OmpBeginSectionsDirective
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE-NOT: ExecutionPartConstruct
+!PARSE-TREE:  OmpEndSectionsDirective
+
+!==============================================================================
+! multiple sections
+!==============================================================================
+!CHECK: !$omp sections
+!$omp sections
+  !CHECK: !$omp section
+  !$omp section
+    !CHECK: CALL F1
+    call F1
+  !CHECK: !$omp section
+  !$omp section
+    !CHECK: CALL F2
+    call F2
+  !CHECK: !$omp section
+  !$omp section
+    !CHECK: CALL F3
+    call F3
+!CHECK: !$omp end sections
+!$omp end sections
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPSectionsConstruct
+!PARSE-TREE:  OmpBeginSectionsDirective
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE-NOT: ExecutionPartConstruct
+!PARSE-TREE:  OmpEndSectionsDirective
+
+!==============================================================================
+! multiple sections with clauses
+!==============================================================================
+!CHECK: !$omp sections PRIVATE(x) FIRSTPRIVATE(y)
+!$omp sections PRIVATE(x) FIRSTPRIVATE(y)
+  !CHECK: !$omp section
+  !$omp section
+    !CHECK: CALL F1
+    call F1
+  !CHECK: !$omp section
+  !$omp section
+    !CHECK: CALL F2
+    call F2
+  !CHECK: !$omp section
+  !$omp section
+    !CHECK: CALL F3
+    call F3
+!CHECK: !$omp end sections NOWAIT
+!$omp end sections NOWAIT
+
+!PARSE-TREE: OpenMPConstruct -> OpenMPSectionsConstruct
+!PARSE-TREE:  OmpBeginSectionsDirective
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE:   OpenMPConstruct -> OpenMPSectionConstruct -> Block
+!PARSE-TREE:    CallStmt
+!PARSE-TREE-NOT: ExecutionPartConstruct
+!PARSE-TREE:  OmpEndSectionsDirective
+
+END subroutine openmp_sections