This is an archive of the discontinued LLVM Phabricator instance.

[flang][openmp] Add General Semantic Checks for Allocate Directive
ClosedPublic

Authored by I_Perry on Nov 10 2020, 6:38 AM.

Details

Summary

This patch adds semantic checks for the General Restrictions of the Allocate Directive.

Since the requires directive is not yet implemented in Flang, the restriction
"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"
will need to be updated at a later time.

A different patch will be made with the Fortran specific restrictions of this directive.

I have used the code from https://reviews.llvm.org/D89395 for the CheckObjectListStructure function.

Diff Detail

Event Timeline

Rin created this revision.Nov 10 2020, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2020, 6:38 AM
Rin requested review of this revision.Nov 10 2020, 6:38 AM
lebedev.ri retitled this revision from Add Semantic Checks for Allocate Directive to [flang] Add Semantic Checks for Allocate Directive.Nov 10 2020, 6:39 AM
Rin retitled this revision from [flang] Add Semantic Checks for Allocate Directive to [flang]Add Semantic Checks for Allocate Directive.Nov 10 2020, 6:40 AM
Rin edited the summary of this revision. (Show Details)
Rin added inline comments.Nov 10 2020, 6:43 AM
flang/test/Semantics/omp-allocate03.f90
16

This test is currently failing as it is not throwing any errors, although one is expected.

Rin updated this revision to Diff 304452.Nov 11 2020, 3:29 AM

Fix failing test

Rin edited the summary of this revision. (Show Details)Nov 11 2020, 6:44 AM

I) Are you handling only a subset of the restrictions here? I see the following in,

  1. General restrictions.

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

  1. Fortran specific restrictions.

-> List items specified in the allocate directive must not have the ALLOCATABLE attribute unless the directive is associated with an allocate statement.
-> List items specified in an allocate directive that is associated with an allocate statement must be variables that are allocated by the allocate statement.
-> Multiple directives can only be associated with an allocate statement if list items are specified on each allocate directive.
-> If a list item has the SAVE attribute, is a common block name, or is declared in the scope of a module, then only predefined memory allocator parameters can be used in the allocator clause.
-> A type parameter inquiry cannot appear in an allocate directive.

Since the requires directive is not there I am assuming that only a subset of the check in the general restrictions can be checked now.
Please provide a clarification saying that only a subset of checks are implemented or implement the above checks.

II) A few files have no newline at end of file.

Rin added a comment.Nov 17 2020, 2:43 AM

I) Are you handling only a subset of the restrictions here? I see the following in,

  1. General restrictions.

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

  1. Fortran specific restrictions.

-> List items specified in the allocate directive must not have the ALLOCATABLE attribute unless the directive is associated with an allocate statement.
-> List items specified in an allocate directive that is associated with an allocate statement must be variables that are allocated by the allocate statement.
-> Multiple directives can only be associated with an allocate statement if list items are specified on each allocate directive.
-> If a list item has the SAVE attribute, is a common block name, or is declared in the scope of a module, then only predefined memory allocator parameters can be used in the allocator clause.
-> A type parameter inquiry cannot appear in an allocate directive.

Since the requires directive is not there I am assuming that only a subset of the check in the general restrictions can be checked now.
Please provide a clarification saying that only a subset of checks are implemented or implement the above checks.

II) A few files have no newline at end of file.

I) So sorry about that. Don't know how I managed to miss all of those. I'll start working on them.
II) My editor seems to be doing that, I'll fix it with the next update.

Rin updated this revision to Diff 307568.Nov 25 2020, 3:12 AM

Add semantic check for allocate directives within target region

Rin retitled this revision from [flang]Add Semantic Checks for Allocate Directive to [flang]Add General Semantic Checks for Allocate Directive.Nov 25 2020, 3:13 AM
Rin edited the summary of this revision. (Show Details)
Rin edited the summary of this revision. (Show Details)Nov 25 2020, 3:22 AM
Rin updated this revision to Diff 311169.Dec 11 2020, 4:46 AM

Rebase with Parser for Allocate Directive patch

flang/include/flang/Parser/parse-tree.h
3807–3808

Can you post another patch without these conflicts?

Rin updated this revision to Diff 311209.Dec 11 2020, 6:17 AM

Get rid of conflicts

Rin updated this revision to Diff 311237.Dec 11 2020, 8:40 AM

Get rid of conflicts

Rin marked an inline comment as done.Dec 11 2020, 8:40 AM

First few comments.

flang/lib/Semantics/check-omp-structure.cpp
475

Can you reuse the following function?
void OmpStructureChecker::CheckIsVarPartOfAnotherVar

954–955

Why are all these removed in this patch?

flang/lib/Semantics/resolve-directives.cpp
121–122

Why is this change in OpenACC required?

462–463

Is it possible to do the semantic checks without these two globals?
If not this should be initialized with braces.

flang/test/Semantics/omp-allocate01.f90
25

Check newline in all test files.

clementval requested changes to this revision.Dec 11 2020, 11:39 AM
clementval added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
954–955

This would break any allowance check on these clauses. I don't see the reason to remove them in this patch.

flang/lib/Semantics/resolve-directives.cpp
121–122

Please revert this change. OpenACC part should not be touch for OpenMP related code.

1018

I guess you can query the current context instead of using this global variable.

1333

I would remove those two globals and for the inTarget you can query the context stack. There is an example in OpenACC (https://github.com/llvm/llvm-project/blob/d02eac0c000984865dd1ce2474715538f8439470/flang/lib/Semantics/check-acc-structure.cpp#L67)

For the hasAllocator you can probably go through the list here to get the information.

1429

Can you reuse the ResolveOmpName function instead of this one?

This revision now requires changes to proceed.Dec 11 2020, 11:39 AM
Rin added inline comments.Dec 14 2020, 9:53 AM
flang/lib/Semantics/check-omp-structure.cpp
475

Yes, this function wasn't yet here when I started working on this patch. I will reuse this function instead.

954–955

I think I was getting some errors about redefinition of those checks. I'll look into this and re add them next update.

flang/lib/Semantics/resolve-directives.cpp
121–122

It's not, I will revert this next update.

flang/test/Semantics/omp-allocate01.f90
25

Will do.

flang/test/Semantics/omp-allocate03.f90
16

I notice here I took care of only the DeclarativeAllocate case and not the Executable one. I will change this in the next update.

Rin updated this revision to Diff 312749.Dec 18 2020, 3:45 AM

Remove unnecessary global variables and add variable check and test for ExecutableAllocateDirective case

Rin marked 7 inline comments as done.Dec 18 2020, 3:53 AM
Rin added inline comments.
flang/lib/Semantics/resolve-directives.cpp
1333

Thanks for your example @clementval. It was really helpful.

1429

I don't see how ResolveOmpName checks the Scope similarly to the way ResolveOmpObjectScope does. I'm not sure how I could reuse it. I'll take another look at it though.

tskeith retitled this revision from [flang]Add General Semantic Checks for Allocate Directive to [flang][openmp] Add General Semantic Checks for Allocate Directive.Dec 18 2020, 7:19 AM
tskeith added a project: Restricted Project.
clementval added inline comments.Dec 18 2020, 7:25 AM
flang/lib/Semantics/resolve-directives.cpp
1429

Fine as well if you cannot reuse it. Can you maybe add a comment on the function to tell how it is different then ResolveOmpName?

Rin changed the edit policy from "All Users" to "Custom Policy".Dec 18 2020, 7:45 AM
Rin changed the edit policy from "Custom Policy" to "All Users".
kiranchandramohan commandeered this revision.Dec 18 2020, 7:46 AM
kiranchandramohan edited reviewers, added: Rin; removed: kiranchandramohan.
kiranchandramohan marked an inline comment as done.

Rebasing on main.

Rin commandeered this revision.Dec 18 2020, 7:48 AM
Rin edited reviewers, added: kiranchandramohan; removed: Rin.
clementval added inline comments.Jan 7 2021, 6:50 AM
flang/lib/Semantics/resolve-directives.cpp
488

Would be even nicer to have a generic function like bool IsNestedInDirective(const llvm::omg::Directive dir) so it can be reused later for other check. Might even be in the common class For OpenACC and OpenMP.

No need to do it in this revision but just keep the idea in mind.

I_Perry commandeered this revision.Mar 26 2021, 7:33 AM
I_Perry added a reviewer: Rin.
I_Perry added a subscriber: I_Perry.

I'll take this patch over from Rin and take it to completion.

I'll take this patch over from Rin and take it to completion.

Thanks!

I_Perry added inline comments.Mar 26 2021, 8:06 AM
flang/lib/Semantics/resolve-directives.cpp
488

I'm looking into making a more generic function for this purpose as part of this patch, do you know if anyone has implemented anything like IsNestedInDirective
already at this point?

I_Perry marked 2 inline comments as done.Mar 26 2021, 8:27 AM
I_Perry updated this revision to Diff 336092.Apr 8 2021, 6:56 AM

This update changes the function IsInsideTargetDirective to the more generic IsNestedInDirective and adds a comment explaining when to use ResolveOmpObjectScope over ResolveOmpName.

Two first comments. I will do a detailed review later.

flang/lib/Semantics/resolve-directives.cpp
1346

This variable is not used and will cause a warning.

1480

I think ResolveOmpObjectScope should only be called if it is an allocate directive. Please reorder the operands of the && operation. The omp-do01.f90 test will also pass.

I_Perry marked 4 inline comments as done.Apr 12 2021, 10:23 AM
I_Perry updated this revision to Diff 336934.Apr 12 2021, 12:22 PM

Adressed comments

Removed an unused variable and changed the order of an && expression, which was causing test omp-do01.f90 to fail.

I_Perry updated this revision to Diff 337126.Apr 13 2021, 6:20 AM

Fixed incorrect formatting.

I_Perry marked 2 inline comments as done.Apr 13 2021, 6:33 AM

LGTM.

@clementval do you have more comments?

flang/lib/Semantics/resolve-directives.cpp
1345

Nit: Can probably use parser::Unwrap on the clauselist to check for the allocator.

1436–1440

Nit: Can ResolveName be used instead of the above 5 lines?

1442

Nit: Add a TODO above to check whether the following code needs to be in ResolveOmpName as well.

clementval accepted this revision.Apr 19 2021, 6:38 PM

LGTM.

@clementval do you have more comments?

LGTM

This revision is now accepted and ready to land.Apr 19 2021, 6:38 PM
I_Perry updated this revision to Diff 339268.Apr 21 2021, 9:15 AM

Patch ready for merge.

@kiranchandramohan @awarzynski This patch has been rebased against main, all tests pass and it is ready for merge.

I_Perry updated this revision to Diff 339662.Apr 22 2021, 8:35 AM

Addressed nit comments.

Addressed nit comments.

Do you have access to land the patch?

I_Perry marked 3 inline comments as done.Apr 22 2021, 8:38 AM

Addressed nit comments.

Do you have access to land the patch?

I do not, however both @awarzynski and @kiranchandramohan have offered to merge this patch (which should be ready now).

I do not, however both @awarzynski and @kiranchandramohan have offered to merge this patch (which should be ready now).

Ok good. I'll let them merge it then.

This revision was landed with ongoing or failed builds.Apr 22 2021, 9:15 AM
This revision was automatically updated to reflect the committed changes.