Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
474

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

953–954

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
953–954

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.

1431

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
474

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

953–954

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.

1431

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
1431

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.

1486

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.

1438–1442

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

1444

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.