Page MenuHomePhabricator

[flang][openmp] Add General Semantic Checks for Allocate Directive
Needs ReviewPublic

Authored by Rin 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
3812–3813

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
193

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

457–458

Why are all these removed in this patch?

flang/lib/Semantics/resolve-directives.cpp
116–117

Why is this change in OpenACC required?

351–352

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
457–458

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
116–117

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

812

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

1099

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.

1198

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
193

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

457–458

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
116–117

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
1099

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

1198

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
1198

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.Thu, Jan 7, 6:50 AM
flang/lib/Semantics/resolve-directives.cpp
369

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.