This is an archive of the discontinued LLVM Phabricator instance.

[flang]Add Parser Support for OpenMP Allocate Directive
ClosedPublic

Authored by Rin on Oct 16 2020, 9:56 AM.

Details

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

Diff Detail

Event Timeline

Rin created this revision.Oct 16 2020, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2020, 9:56 AM
Rin requested review of this revision.Oct 16 2020, 9:56 AM
Rin updated this revision to Diff 298658.Oct 16 2020, 9:59 AM

clang-format

Rin retitled this revision from [flang]Add Parser Support for Allocate Directive to [flang]Add Parser Support for OpenMP Allocate Directive.Oct 16 2020, 10:00 AM
Rin added a project: Restricted Project.
Rin added inline comments.Oct 16 2020, 10:03 AM
flang/include/flang/Parser/parse-tree.h
3623

The Allocate Directive can take the Allocator Clause as its only clause, but that clause doesn't seem to have been implemented yet. Should I add it myself before going forward with this patch or can this be done at a later time?

Harbormaster completed remote builds in B75323: Diff 298658.

Can you provide more information about the allocate directive in the summary/description of this patch?
-> Point to the section in the standard.
-> Explain that the standard specifies a declarative and executable version.

Also add tests for the executable version as well. Have both of them in the same test to show that the parser is not confused.
Add unparse tests for both versions.

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

Yes, add that also.

Rin edited the summary of this revision. (Show Details)Oct 19 2020, 4:09 AM
Rin updated this revision to Diff 299364.Oct 20 2020, 8:22 AM

Add Allocator Clause, Additional Tests and make the OmpObjectList optional for the Executable Allocate Clause

Rin marked an inline comment as done.Oct 20 2020, 8:24 AM
clementval requested changes to this revision.Oct 23 2020, 7:55 AM
clementval added inline comments.
flang/include/flang/Parser/parse-tree.h
3626

You should use the OmpClauseList and let the semantic check do the work. Otherwise each directive will have there own parser for theirs associated directive and this duplicates the code a lot.

OMP.td is already good for this directive.

def OMP_Allocate : Directive<"allocate"> {
  let allowedClauses = [
    VersionedClause<OMPC_Allocator>
  ];
}
flang/lib/Parser/openmp-parsers.cpp
465

You should re-use OmpClause parser instead of duplicating the parsing. If it's not in the OmpClause parser, just add it there.

479

Same here for the parser.

This revision now requires changes to proceed.Oct 23 2020, 7:55 AM
Rin updated this revision to Diff 300983.Oct 27 2020, 7:23 AM

Address review comments

Rin marked 3 inline comments as done.Oct 27 2020, 7:24 AM
Rin added inline comments.
flang/include/flang/Parser/parse-tree.h
3626

Thanks for that. I'll make sure to do so.

clementval added inline comments.Oct 27 2020, 8:43 AM
flang/include/flang/Parser/dump-parse-tree.h
549

Any reason to keep this?

557

Any reason to keep this?

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

Newline.

Rin marked an inline comment as done.Oct 27 2020, 8:46 AM
Rin added inline comments.
flang/include/flang/Parser/dump-parse-tree.h
549

None other than the fact that I didn't notice it when I was cleaning up the code. Really sorry about that. Will remove it straight away. Same for the other one.

Rin updated this revision to Diff 301028.Oct 27 2020, 9:14 AM

Remove unnecessary comment and add newline to test

Rin marked 3 inline comments as done.Oct 27 2020, 9:15 AM
Rin added a comment.Nov 11 2020, 6:38 AM

Does this look okay @clementval ?

kiranchandramohan requested changes to this revision.Nov 16 2020, 10:43 AM
kiranchandramohan added inline comments.
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?

This revision now requires changes to proceed.Nov 16 2020, 10:43 AM
Rin updated this revision to Diff 306655.Nov 20 2020, 4:07 AM

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

Rin marked 2 inline comments as done.Nov 20 2020, 4:08 AM
clementval added inline comments.Nov 20 2020, 8:08 AM
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 added inline comments.Nov 20 2020, 8:11 AM
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 added inline comments.Nov 20 2020, 8:13 AM
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 added inline comments.Nov 24 2020, 8:34 AM
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 added inline comments.Nov 24 2020, 8:48 AM
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 updated this revision to Diff 307892.Nov 26 2020, 9:03 AM

Remove unnecessary node

clementval accepted this revision.Nov 30 2020, 11:23 AM

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.

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 added inline comments.Dec 1 2020, 2:37 AM
flang/lib/Semantics/check-omp-structure.h
146

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

Rin updated this revision to Diff 308616.Dec 1 2020, 4:22 AM

Order Enter clase function in alphabetical order

Rin marked an inline comment as done.Dec 1 2020, 4:24 AM

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.

sameeranjoshi added a project: Restricted Project.Dec 3 2020, 5:06 AM
clementval added inline comments.Dec 3 2020, 6:07 AM
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 added inline comments.Dec 3 2020, 6:13 AM
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 added inline comments.Dec 3 2020, 6:14 AM
flang/lib/Parser/openmp-parsers.cpp
462

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

Rin updated this revision to Diff 309847.Dec 7 2020, 2:29 AM

Remove white spaces

Rin marked 9 inline comments as done.Dec 7 2020, 2:29 AM
Rin added inline comments.
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 added inline comments.Dec 8 2020, 6:23 AM
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 added inline comments.Dec 8 2020, 8:25 AM
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 added inline comments.Dec 8 2020, 8:36 AM
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 updated this revision to Diff 310240.Dec 8 2020, 9:10 AM

Add DeclarativeAllocateDirective as a list within the ExecutableAllocateDirective construct

Rin marked 6 inline comments as done.Dec 8 2020, 9:12 AM
sameeranjoshi accepted this revision.Dec 10 2020, 2:01 AM

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 added inline comments.Dec 10 2020, 2:10 AM
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 updated this revision to Diff 310842.Dec 10 2020, 4:37 AM

Add comment

Rin marked an inline comment as done.Dec 10 2020, 4:38 AM

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
This revision is now accepted and ready to land.Dec 10 2020, 7:58 AM
This revision was landed with ongoing or failed builds.Dec 10 2020, 8:25 AM
This revision was automatically updated to reflect the committed changes.