[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:3619
+// 2.11.3 allocate -> ALLOCATE [(variable-name-list)] [clause]
+//        allocate-statement
+//        clause -> allocator-clause
----------------
sameeranjoshi wrote:
> Rin wrote:
> > sameeranjoshi wrote:
> > > Are you missing `OpenMPDeclarativeAllocate` in comments?
> > These are the comments for the OpenMPExecutableAllocate so I didn't think it necessary to add here the case where the Allocate Directive is Declarative.
> I meant where is a reference for `std::optional<OpenMPDeclarativeAllocate>` in the comments ?
> ```
> // 2.11.3 allocate -> ALLOCATE [(variable-name-list)] [clause]
> //                             [ALLOCATE [(variable-name-list)] [clause] [...]]
> //                             allocate-statement
> //            clause -> allocator-clause
> 
> ```
> As there needs a stricter differentiation between both allocate directives to identify them.
Oh, right, I misunderstood. I'll add those in the next update.


================
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:
> Rin wrote:
> > 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.
> > 
> You got it correct, so why isn't 
> `std::optional<OpenMPDeclarativeAllocate>` in `OpenMPExecutableAllocate` not a std::list (`std::optional<std::list<OpenMPDeclarativeAllocate>>`)?
I'll add it as a list in the next update. You're right. Sorry, I misread your question in the beginning. 


================
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,
----------------
sameeranjoshi wrote:
> Rin wrote:
> > Rin wrote:
> > > 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.
> > Oh yeah, I forgot about that, you're right.
> Hmm the restriction makes me think that the standard there is some ambiguity.
> I am not sure what to do with such case.
If the clause is required, I can add it in the TableGen file like Valentin mentioned. I'm not sure what the best course of action would be in this case, so I'm open to suggestions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89562