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

[sameeran joshi via Phabricator via llvm-commits]

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


================
Comment at: flang/include/flang/Parser/parse-tree.h:3625
+  std::tuple<Verbatim, std::optional<OmpObjectList>, OmpClauseList,
+      std::optional<OpenMPDeclarativeAllocate>, Statement<AllocateStmt>>
+      t;
----------------
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>>`)?


================
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,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89562