[PATCH] D89562: [flang]Add Parser Support for OpenMP Allocate Directive

[Irina Dobrescu via Phabricator via llvm-commits]

Rin added inline comments.


================
Comment at: flang/include/flang/Parser/parse-tree.h:3625
+  std::tuple<Verbatim, std::optional<OmpObjectList>, OmpClauseList,
+      std::optional<OpenMPDeclarativeAllocate>, Statement<AllocateStmt>>
+      t;
----------------
sameeranjoshi wrote:
> Correct me if my interpretation from standard is wrong.
> ```
> !$omp allocate[(list)] clause
> [!$omp allocate(list)clause[...]]
> allocate statement
> ```
> the 
> `std::optional<OpenMPDeclarativeAllocate>` seem to be a list mentioned by elipsis, or is it that the there should be a list of clauses?
> 
> Althought there are tests(`omp-allocate-directive.f90` line 19-23) which have a list of `OpenMPDeclarativeAllocate` for an `OpenMPExecutableAllocate ` test.
I'm not sure if I'm understanding the question correctly, but if you're asking about the (list), that refers to list-items. 
What I get from this:

!$omp allocate[(list)] clause
[!$omp allocate(list)clause[...]]
allocate statement

Particularly this part: [!$omp allocate(list)clause[...]]

Is that an OpenMPExecutableAllocate can be followed by an optional list of OpenMPDeclarativeAllocate.



================
Comment at: flang/lib/Parser/openmp-parsers.cpp:457
+    sourced(construct<OpenMPExecutableAllocate>(verbatim("ALLOCATE"_tok),
+        maybe(parenthesized(Parser<OmpObjectList>{})), Parser<OmpClauseList>{},
+        maybe(Parser<OpenMPDeclarativeAllocate>{}) / endOmpLine,
----------------
clementval wrote:
> sameeranjoshi wrote:
> > In case of `OpenMPExecutableAllocate` are the clauses mandatory as the standard doesn't seem to have square brackets around them?
> > Or that's wrongly mentioned in standard.
> > 
> > As `OmpClauseList` is a list it can be optional.
> > ```
> > !$omp allocate[(list)] clause
> > [!$omp allocate[(list)] clause
> > [...]]
> > ```
> > 
> > So a test case like
> > ```
> >   !$omp allocate(a, b)
> >      allocate ( darray(a, b) )
> > ```
> > becomes invalid in that case.
> If there are required clauses they are set in the TableGen file. `OmpClauseList` should be used in preference of a stricter parser. 
Hmm, I'm not too sure about this one. You're right that there are no square brackets, but then there are restrictions  such as this one: 

allocate directives that appear in a target region must specify an allocator clause unless a requires directive with the dynamic_allocators clause is present in the same compilation unit.

Which specify when an allocator clause needs to be present on a directive. So maybe they are still optional? I can make them required if my assumption is wrong.


================
Comment at: flang/lib/Parser/openmp-parsers.cpp:474
+        parenthesized(Parser<OmpObjectList>{}), Parser<OmpClauseList>{})) /
+    lookAhead(endOmpLine / !statement(allocateStmt)))
+
----------------
clementval wrote:
> sameeranjoshi wrote:
> > Does a test with no newline and an allocateStmt fall under `OpenMPDeclarativeAllocate`  as per current implementations?
> > ```
> > !$omp allocate(x, y) allocator(omp_default_mem_alloc) allocate( darray(x,y) )
> > ```
> I don't think the `allocate` statement in that case will be handle by the fortran parser. The whole line is considered as a "comment" so the Fortran parser will not try to infer what is inside of it. 
No, I think that would be a comment since the allocateStmt is a Fortran Statement rather than an OpenMP one, so the newline is required.


================
Comment at: flang/lib/Parser/unparse.cpp:2296
+    Put("\n");
+    EndOpenMP();
+    Walk(std::get<Statement<AllocateStmt>>(x.t));
----------------
sameeranjoshi wrote:
> Why is there no `Walk` for `std::optional<OpenMPDeclarativeAllocate>` as parse-tree.h shows
> ```
>    std::tuple<Verbatim, std::optional<OmpObjectList>, OmpClauseList,
>        std::optional<OpenMPDeclarativeAllocate>, Statement<AllocateStmt>>
>        t;
> ``` 
Oh, the unparsing of OpenMPDeclarativeAllocate is done by the function which handles OpenMPDeclarativeAllocate.


================
Comment at: flang/lib/Semantics/check-omp-structure.h:107
   void Leave(const parser::OpenMPDeclareSimdConstruct &);
+  void Enter(const parser::OpenMPDeclarativeAllocate &);
+  void Leave(const parser::OpenMPDeclarativeAllocate &);
----------------
sameeranjoshi wrote:
> The commit title mentions this to be a parser change, are these semantic checks necessary with this patch?
Yes, because the directive context needs to be pushed using the PushContextAndClauseSets() function, otherwise there is an error. So these semantic checks are necessary in this patch.


================
Comment at: flang/test/Parser/omp-allocate-unparse.f90:7
+
+real, dimension (:,:), allocatable :: darray    
+integer :: a, b, m, n, t, x, y, z
----------------
sameeranjoshi wrote:
> I see whitespaces at the end.
I will take care of those in the next update. Sorry about that. Failed to notice this detail.


================
Comment at: flang/test/Semantics/omp-allocate-directive.f90:16
+
+!$omp allocate(a, b) allocator(omp_default_mem_alloc)
+    allocate ( darray(a, b) ) 
----------------
sameeranjoshi wrote:
> Is a test allowed with multiple clauses or will it be handled in semantic phase?
> ```
> !$omp allocate(x, y) allocator(omp_default_mem_alloc) allocator(omp_default_mem_alloc)
> ```
This will be handled in the semantics patch. It's not allowed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89562