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

[Irina Dobrescu via Phabricator via llvm-commits]

Rin added inline comments.


================
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:
> > > 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.
> > I'm really unsure here, because the TableGen refers to both the Declarative and Executable case. So if I make the allocator clause required, then it will be required for both cases. And yeah, the standard is a bit ambiguous, so I'm quite unsure here.
> Seeing the ambiguity in standard and restrictions in semantics, looks like we should go with OmpClauseList.
> 
Alright.


================
Comment at: flang/lib/Parser/type-parsers.h:94
 constexpr Parser<AssignmentStmt> assignmentStmt; // R1032
+constexpr Parser<AllocateStmt> allocateStmt;
 constexpr Parser<PointerAssignmentStmt> pointerAssignmentStmt; // R1033
----------------
sameeranjoshi wrote:
> You missed `// R927` at the end.
Thanks for pointing it out. I'll add it before I push this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89562