[flang] OpenMP allocate directive parse tree fix

[elmcdonough]

Summary

Addresses the same issue as the following abandoned revision: D104391.

Rewrite leading declarative allocations so they are nested within their respective executable allocate directive

Original:

ExecutionPartConstruct -> OpenMPDeclarativeAllocate
ExecutionPartConstruct -> OpenMPDeclarativeAllocate
ExecutionPartConstruct -> OpenMPExecutableAllocate

After rewriting:

ExecutionPartConstruct -> OpenMPExecutableAllocate
| ExecutionPartConstruct -> OpenMPDeclarativeAllocate
| ExecutionPartConstruct -> OpenMPDeclarativeAllocate

Co-authored-by: Isaac Perry <[e-mail]>

[elmcdonough]

Note: Sometimes declarative OpenMP directives aren't able to be read when there isn't a ExecutionPartConstruct to end the SpecificationPart parsing. In these cases, declarative allocation directives are appended to the end of the SpecificationPart instead of at the beginning of the ExecutionPart. I have local changes that address this, but I excluded them because they would cause the XFAIL test test/Semantics/OpenMP/allocate06.f90 to succeed.

If this is a whitespace sensitive issue, it might require changing how the directive grammar outlined in lib/Parser/openmp-parsers.cpp.

[kiranchandramohan]

Thanks @elmcdonough for the patch.

Did you mean this does the same as D104391? Since the change is motivated by changes in the other patches, could you add the other authors as co-authors?
Could you expand the Summary to describe the transformation?
Could you add more tests?

[elmcdonough]

Added another test and updated the patch description.

I'm not sure how to add co-authors in Phabricator, but I can make sure to add @I_Perry as a co-author in the staged commit. I would just need their GitHub username. Is there a Phabricator feature that allows me to directly add another author to this revision?

[elmcdonough]

Pinging this

[kiranchandramohan]

Thanks @elmcdonough. I am having a look at the patch today.

Regarding adding co-author: You can probably just add
Co-authored-by: Isaac Perry <[e-mail]>

[kiranchandramohan]

Nice. Looks mostly fine. I have some comments about the test. Could you fix?
Conditionally accepting subject to fixing the tests.

flang/test/Parser/OpenMP/allocate-tree-spec-part.f90

13

Shouldn't z have to be allocated in the enclosed allocate statement for this test to be valid?

14

What is a here, it is not declared?

flang/test/Parser/OpenMP/allocate-tree.f90

15

z has to be in the enclosed allocate statement.

16

What is a here, it is not declared?

[elmcdonough]

Updated the tests to be semantically sound and added an unparse test.

[elmcdonough]

@kiranchandramohan Please let me know if these changes look good when you get the chance.

[kiranchandramohan]

LG.

[elmcdonough]

I apologize for the amount of reverts. Arcanist kept stripping out the co-author information when I landed my changes. My latest commit contains the co-author information (Co-authored-by: Isaac Perry <[e-mail]>) in the commit message body, but Isaac doesn't show up as a co-author in the UI because of formatting restrictions:

Addresses the same issue as the following abandoned revision: D104391.

Rewrite leading declarative allocations so they are nested within their respective executable allocate directive

Original:
ExecutionPartConstruct -> OpenMPDeclarativeAllocate
ExecutionPartConstruct -> OpenMPDeclarativeAllocate
ExecutionPartConstruct -> OpenMPExecutableAllocate

After rewriting:
ExecutionPartConstruct -> OpenMPExecutableAllocate
| ExecutionPartConstruct -> OpenMPDeclarativeAllocate
| ExecutionPartConstruct -> OpenMPDeclarativeAllocate

Co-authored-by: Isaac Perry <[e-mail]> <--- Where the co-author line is

Reviewed By: kiranchandramohan

Differential Revision: https://reviews.llvm.org/D148409


 <--- Where the co-author line should be to show up in UI

I would like to emphasize that Isaac is credited in the commit message. I just wanted to make sure this is okay @kiranchandramohan

[clementval]

In D148409#4323066, @elmcdonough wrote:

I apologize for the amount of reverts. Arcanist kept stripping out the co-author information when I landed my changes. My latest commit contains the co-author information (Co-authored-by: Isaac Perry <[e-mail]>) in the commit message body, but Isaac doesn't show up as a co-author in the UI because of formatting restrictions:

Addresses the same issue as the following abandoned revision: D104391.

Rewrite leading declarative allocations so they are nested within their respective executable allocate directive

Original:
ExecutionPartConstruct -> OpenMPDeclarativeAllocate
ExecutionPartConstruct -> OpenMPDeclarativeAllocate
ExecutionPartConstruct -> OpenMPExecutableAllocate

After rewriting:
ExecutionPartConstruct -> OpenMPExecutableAllocate
| ExecutionPartConstruct -> OpenMPDeclarativeAllocate
| ExecutionPartConstruct -> OpenMPDeclarativeAllocate

Co-authored-by: Isaac Perry <[e-mail]> <--- Where the co-author line is

Reviewed By: kiranchandramohan

Differential Revision: https://reviews.llvm.org/D148409


 <--- Where the co-author line should be to show up in UI

I would like to emphasize that Isaac is credited in the commit message. I just wanted to make sure this is okay @kiranchandramohan

If you want the co author line to be the last you have to land it by hand (without arcanist)

[elmcdonough]

Thank you @clementval, that did the trick. I apologize for this whole situation.

flang/lib/Parser/unparse.cpp

void Unparse(const OmpAtomicWrite &x) {
     Put("\n");
     EndOpenMP();
     Walk(std::get<Statement<AssignmentStmt>>(x.t));
     BeginOpenMP();
     Walk(std::get<std::optional<OmpEndAtomic>>(x.t), "!$OMP END ATOMIC\n");
     EndOpenMP();
   }
   void Unparse(const OpenMPExecutableAllocate &x) {
+    const auto &fields =
+        std::get<std::optional<std::list<parser::OpenMPDeclarativeAllocate>>>(
+            x.t);
+    if (fields) {
+      for (const auto &decl : *fields) {
+        Walk(decl);
+      }
+    }
     BeginOpenMP();
     Word("!$OMP ALLOCATE");
     Walk(" (", std::get<std::optional<OmpObjectList>>(x.t), ")");
     Walk(std::get<OmpClauseList>(x.t));
     Put("\n");
     EndOpenMP();
     Walk(std::get<Statement<AllocateStmt>>(x.t));
   }

flang/lib/Semantics/canonicalize-omp.cpp

 #include "flang/Parser/parse-tree-visitor.h"
 
 // After Loop Canonicalization, rewrite OpenMP parse tree to make OpenMP
 // Constructs more structured which provide explicit scopes for later
 // structural checks and semantic analysis.
 //   1. move structured DoConstruct and OmpEndLoopDirective into
 //      OpenMPLoopConstruct. Compilation will not proceed in case of errors
 //      after this pass.
-//   2. TBD
+//   2. Associate declarative OMP allocation directives with their
+//      respective executable allocation directive
+//   3. TBD
 namespace Fortran::semantics {
 
 using namespace parser::literals;
 
 class CanonicalizationOfOmp {
 public:
   template <typename T> bool Pre(T &) { return true; }
   template <typename T> void Post(T &) {}
for (auto it{block.begin()}; it != block.end(); ++it) {
         messages_.Say(dir.source,
             "The %s directive must follow the DO loop associated with the "
             "loop construct"_err_en_US,
             parser::ToUpperCaseLetters(dir.source.ToString()));
       }
     } // Block list
   }
 
+  void Post(parser::ExecutionPart &body) { RewriteOmpAllocations(body); }
+
 private:
   template <typename T> T *GetConstructIf(parser::ExecutionPartConstruct &x) {
     if (auto *y{std::get_if<parser::ExecutableConstruct>(&x.u)}) {
       if (auto *z{std::get_if<common::Indirection<T>>(&y->u)}) {
         return &z->value();
       }
     }
     return nullptr;
   }
 
+  template <typename T> T *GetOmpIf(parser::ExecutionPartConstruct &x) {
+    if (auto *construct{GetConstructIf<parser::OpenMPConstruct>(x)}) {
+      if (auto *omp{std::get_if<T>(&construct->u)}) {
+        return omp;
+      }
+    }
+    return nullptr;
+  }
+
   void RewriteOpenMPLoopConstruct(parser::OpenMPLoopConstruct &x,
       parser::Block &block, parser::Block::iterator it) {
     // Check the sequence of DoConstruct and OmpEndLoopDirective
     // in the same iteration
     //
     // Original:
     //   ExecutableConstruct -> OpenMPConstruct -> OpenMPLoopConstruct
     //     OmpBeginLoopDirective
if (++nextIt != block.end()) {
         return; // found do-loop
       }
     }
     messages_.Say(dir.source,
         "A DO loop must follow the %s directive"_err_en_US,
         parser::ToUpperCaseLetters(dir.source.ToString()));
   }
 
+  void RewriteOmpAllocations(parser::ExecutionPart &body) {
+    // Rewrite leading declarative allocations so they are nested
+    // within their respective executable allocate directive
+    //
+    // Original:
+    //   ExecutionPartConstruct -> OpenMPDeclarativeAllocate
+    //   ExecutionPartConstruct -> OpenMPDeclarativeAllocate
+    //   ExecutionPartConstruct -> OpenMPExecutableAllocate
+    //
+    // After rewriting:
+    //   ExecutionPartConstruct -> OpenMPExecutableAllocate
+    //     ExecutionPartConstruct -> OpenMPDeclarativeAllocate
+    //     ExecutionPartConstruct -> OpenMPDeclarativeAllocate
+    for (auto it = body.v.rbegin(); it != body.v.rend();) {
+      if (auto *exec = GetOmpIf<parser::OpenMPExecutableAllocate>(*(it++))) {
+        parser::OpenMPDeclarativeAllocate *decl;
+        std::list<parser::OpenMPDeclarativeAllocate> subAllocates;
+        while (it != body.v.rend() &&
+            (decl = GetOmpIf<parser::OpenMPDeclarativeAllocate>(*it))) {
+          subAllocates.push_front(std::move(*decl));
+          it = decltype(it)(body.v.erase(std::next(it).base()));
+        }
+        if (!subAllocates.empty()) {
+          std::get<std::optional<std::list<parser::OpenMPDeclarativeAllocate>>>(
+              exec->t) = {std::move(subAllocates)};
+        }
+      }
+    }
+  }
+
   parser::Messages &messages_;
 };
 
 bool CanonicalizeOmp(parser::Messages &messages, parser::Program &program) {
   CanonicalizationOfOmp omp{messages};
   Walk(program, omp);
   return !messages.AnyFatalError();
 }
 } // namespace Fortran::semantics

flang/lib/Semantics/resolve-directives.cpp

common::visit(
                     CheckObjectInNamelist(*name, *symbol, ompFlag);
                   }
 
                   if (ompFlag == Symbol::Flag::OmpAllocate) {
                     AddAllocateName(name);
                   }
                 }
                 if (ompFlag == Symbol::Flag::OmpDeclarativeAllocateDirective &&
-                    IsAllocatable(*symbol)) {
+                    IsAllocatable(*symbol) &&
+                    !IsNestedInDirective(llvm::omp::Directive::OMPD_allocate)) {
                   context_.Say(designator.source,
                       "List items specified in the ALLOCATE directive must not "
                       "have the ALLOCATABLE attribute unless the directive is "
                       "associated with an ALLOCATE statement"_err_en_US);
                 }
                 if ((ompFlag == Symbol::Flag::OmpDeclarativeAllocateDirective ||
                         ompFlag ==
                             Symbol::Flag::OmpExecutableAllocateDirective) &&

flang/test/Parser/OpenMP/allocate-tree-spec-part.f90

+! RUN: %flang_fc1 -fopenmp -fdebug-dump-parse-tree %s | FileCheck %s
+! Ensures associated declarative OMP allocations in the specification
+! part are kept there
+
+program allocate_tree
+    use omp_lib
+    integer, allocatable :: w, xarray(:), zarray(:, :)
+    integer :: f
+!$omp allocate(f) allocator(omp_default_mem_alloc)
+    f = 2
+!$omp allocate(w) allocator(omp_const_mem_alloc)
+!$omp allocate(xarray) allocator(omp_large_cap_mem_alloc)
+!$omp allocate(zarray) allocator(omp_default_mem_alloc)
+!$omp allocate
+    allocate (w, xarray(4), zarray(5, f))
+end program allocate_tree
+
+!CHECK: | | DeclarationConstruct -> SpecificationConstruct -> OpenMPDeclarativeConstruct -> OpenMPDeclarativeAllocate
+!CHECK-NEXT: | | | Verbatim
+!CHECK-NEXT: | | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'f'
+!CHECK-NEXT: | | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr =
+!CHECK-NEXT: | | | | Designator -> DataRef -> Name =
+!CHECK-NEXT: | ExecutionPart -> Block
+!CHECK-NEXT: | | ExecutionPartConstruct -> ExecutableConstruct -> ActionStmt -> AssignmentStmt = 'f=2_4'
+!CHECK-NEXT: | | | Variable = 'f'
+!CHECK-NEXT: | | | | Designator -> DataRef -> Name = 'f'
+!CHECK-NEXT: | | | Expr = '2_4'
+!CHECK-NEXT: | | | | LiteralConstant -> IntLiteralConstant = '2'
+!CHECK-NEXT: | | ExecutionPartConstruct -> ExecutableConstruct -> OpenMPConstruct -> OpenMPExecutableAllocate
+!CHECK-NEXT: | | | Verbatim
+!CHECK-NEXT: | | | OmpClauseList ->
+!CHECK-NEXT: | | | OpenMPDeclarativeAllocate
+!CHECK-NEXT: | | | | Verbatim
+!CHECK-NEXT: | | | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'w'
+!CHECK-NEXT: | | | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr =
+!CHECK-NEXT: | | | | | Designator -> DataRef -> Name =
+!CHECK-NEXT: | | | OpenMPDeclarativeAllocate
+!CHECK-NEXT: | | | | Verbatim
+!CHECK-NEXT: | | | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'xarray'
+!CHECK-NEXT: | | | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr =
+!CHECK-NEXT: | | | | | Designator -> DataRef -> Name =
+!CHECK-NEXT: | | | OpenMPDeclarativeAllocate
+!CHECK-NEXT: | | | | Verbatim
+!CHECK-NEXT: | | | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'zarray'
+!CHECK-NEXT: | | | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr =
+!CHECK-NEXT: | | | | | Designator -> DataRef -> Name =
+!CHECK-NEXT: | | | AllocateStmt

flang/test/Parser/OpenMP/allocate-tree.f90

+! RUN: %flang_fc1 -fopenmp -fdebug-dump-parse-tree %s | FileCheck %s
+! RUN: %flang_fc1 -fopenmp -fdebug-unparse %s | FileCheck %s --check-prefix="UNPARSE"
+! Ensures associated declarative OMP allocations are nested in their
+! corresponding executable allocate directive
+
+program allocate_tree
+    use omp_lib
+    integer, allocatable :: w, xarray(:), zarray(:, :)
+    integer :: z, t
+    t = 2
+    z = 3
+!$omp allocate(w) allocator(omp_const_mem_alloc)
+!$omp allocate(xarray) allocator(omp_large_cap_mem_alloc)
+!$omp allocate(zarray) allocator(omp_default_mem_alloc)
+!$omp allocate
+    allocate(w, xarray(4), zarray(t, z))
+end program allocate_tree
+
+!CHECK: | | ExecutionPartConstruct -> ExecutableConstruct -> OpenMPConstruct -> OpenMPExecutableAllocate
+!CHECK-NEXT: | | | Verbatim
+!CHECK-NEXT: | | | OmpClauseList ->
+!CHECK-NEXT: | | | OpenMPDeclarativeAllocate
+!CHECK-NEXT: | | | | Verbatim
+!CHECK-NEXT: | | | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'w'
+!CHECK-NEXT: | | | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr =
+!CHECK-NEXT: | | | | | Designator -> DataRef -> Name =
+!CHECK-NEXT: | | | OpenMPDeclarativeAllocate
+!CHECK-NEXT: | | | | Verbatim
+!CHECK-NEXT: | | | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'xarray'
+!CHECK-NEXT: | | | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr =
+!CHECK-NEXT: | | | | | Designator -> DataRef -> Name =
+!CHECK-NEXT: | | | OpenMPDeclarativeAllocate
+!CHECK-NEXT: | | | | Verbatim
+!CHECK-NEXT: | | | | OmpObjectList -> OmpObject -> Designator -> DataRef -> Name = 'zarray'
+!CHECK-NEXT: | | | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr =
+!CHECK-NEXT: | | | | | Designator -> DataRef -> Name =
+!CHECK-NEXT: | | | AllocateStmt
+
+!UNPARSE: !$OMP ALLOCATE (w) ALLOCATOR(1_4)
+!UNPARSE-NEXT: !$OMP ALLOCATE (xarray) ALLOCATOR(1_4)
+!UNPARSE-NEXT: !$OMP ALLOCATE (zarray) ALLOCATOR(1_4)
+!UNPARSE-NEXT: !$OMP ALLOCATE
+!UNPARSE-NEXT: ALLOCATE(w, xarray(4_4), zarray(t,z))