[flang]Add Parser Support for OpenMP Allocate Directive

[Rin]

Summary

This patch adds the Allocate Directive to the parser definition.

The OpenMP standard specifies that that the allocate directive is a declarative directive if it is not associated with an allocation statement, in which case it is executable.

OpenMP 5.0 standard specification: Section 2.11.3 Allocate Directive https://www.openmp.org/wp-content/uploads/OpenMP-API-Specification-5.0.pdf

[kiranchandramohan]

flang/lib/Parser/openmp-parsers.cpp

459

The standard supports a list of openmp allocates associated with a single fortran allocate. Can you extend this to include the list?

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

flang/test/Parser/omp-allocate-unparse.f90

27

This check is probably not sufficient. Can you check for all the openmp allocates and the fortran allocate statements?

[Rin]

Add support for list of OMP allocates and add additional checks in tests.

[clementval]

flang/include/flang/Parser/parse-tree.h

3625

Is there a good reason to have separate nodes (OpenMPDeclarativeAllocate, OpenMPExecutableAllocateList)? Both node have the same content.

[Rin]

flang/include/flang/Parser/parse-tree.h

3625

OpenMPDeclarativeAllocate is part of the Declarative construct, whereas OpenMPExecutableAllocateList is part of the Executable construct. I can't use OpenMPDeclarativeAllocate as an executable construct is expected.

[Rin]

flang/include/flang/Parser/parse-tree.h

3625

I had the same idea to use the OpenMPDeclarativeAllocate, but failed to find a way to make it work unfortunately :/

[clementval]

flang/include/flang/Parser/parse-tree.h

3625

You should be able to use the same node in the two variants. What kind of problem did you face?

[Rin]

flang/include/flang/Parser/parse-tree.h

3625

My issue was that OpenMPDeclarativeAllocate is a Declarative construct and an Executable construct was expected. I could add OpenMPDeclarativeAllocate to the Executable construct variant and see if that works

[Rin]

Remove unnecessary node

[clementval]

Just have a small comment. Otherwise I think it looks good to me. Please wait for the approval of someone on the OpenMP side.

flang/lib/Semantics/check-omp-structure.h

146

Can you move this up. I think the enter clause function are listed by alphabetical order.

[sameeranjoshi]

I can review it from start as I am now more familiar with the files in this patch.
Please give me a day or so to revert back.
If it's approved from someone else in OpenMP you can go ahead without waiting from my end.

[Rin]

flang/lib/Semantics/check-omp-structure.h

146

Yeah, sure thing, I'll do so right away.

[Rin]

Order Enter clase function in alphabetical order

[sameeranjoshi]

Thanks for working on this, I have mostly a few queries and some suggestions.

flang/include/flang/Parser/parse-tree.h

3619

Are you missing OpenMPDeclarativeAllocate in comments?

3625

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.

flang/lib/Parser/openmp-parsers.cpp

462

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.

479

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) )

flang/lib/Parser/unparse.cpp

2296

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;

flang/lib/Semantics/check-omp-structure.h

107

The commit title mentions this to be a parser change, are these semantic checks necessary with this patch?

flang/test/Parser/omp-allocate-unparse.f90

8

I see whitespaces at the end.

19

whitespace

21

whitespace.

23

whitespace

29

whitespace

flang/test/Semantics/omp-allocate-directive.f90

9

whitespace at end.

15

whitespace at end.

17

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)

18

whitespace at end.

24

whitespace at end.

[clementval]

flang/lib/Parser/openmp-parsers.cpp

462

If there are required clauses they are set in the TableGen file. OmpClauseList should be used in preference of a stricter parser.

479

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.

[Rin]

flang/include/flang/Parser/parse-tree.h

3625

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.

flang/lib/Parser/openmp-parsers.cpp

462

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.

479

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.

flang/lib/Parser/unparse.cpp

2296

Oh, the unparsing of OpenMPDeclarativeAllocate is done by the function which handles OpenMPDeclarativeAllocate.

flang/lib/Semantics/check-omp-structure.h

107

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.

flang/test/Parser/omp-allocate-unparse.f90

8

I will take care of those in the next update. Sorry about that. Failed to notice this detail.

flang/test/Semantics/omp-allocate-directive.f90

17

This will be handled in the semantics patch. It's not allowed.

[Rin]

flang/lib/Parser/openmp-parsers.cpp

462

Oh yeah, I forgot about that, you're right.

[Rin]

Remove white spaces

[Rin]

flang/include/flang/Parser/parse-tree.h

3619

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.

[sameeranjoshi]

flang/include/flang/Parser/parse-tree.h

3619

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.

3625

You got it correct, so why isn't
std::optional<OpenMPDeclarativeAllocate> in OpenMPExecutableAllocate not a std::list (std::optional<std::list<OpenMPDeclarativeAllocate>>)?

flang/lib/Parser/openmp-parsers.cpp

462

Hmm the restriction makes me think that the standard there is some ambiguity.
I am not sure what to do with such case.

[Rin]

flang/include/flang/Parser/parse-tree.h

3619

Oh, right, I misunderstood. I'll add those in the next update.

3625

I'll add it as a list in the next update. You're right. Sorry, I misread your question in the beginning.

flang/lib/Parser/openmp-parsers.cpp

462

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.

[Rin]

flang/lib/Parser/openmp-parsers.cpp

462

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.

[Rin]

Add DeclarativeAllocateDirective as a list within the ExecutableAllocateDirective construct

[sameeranjoshi]

Thanks and LGTM.

flang/lib/Parser/openmp-parsers.cpp

462

Seeing the ambiguity in standard and restrictions in semantics, looks like we should go with OmpClauseList.

flang/lib/Parser/type-parsers.h

95

You missed // R927 at the end.

[Rin]

flang/lib/Parser/openmp-parsers.cpp

462

Alright.

flang/lib/Parser/type-parsers.h

95

Thanks for pointing it out. I'll add it before I push this.

[Rin]

Add comment

[kiranchandramohan]

LGTM.

For semantics should have a check where the following case is caught.
!$omp allocate[(list)] clause
!$omp allocate(list)
!$omp allocate(list)

allocate statement