Page MenuHomePhabricator

[flang]Add Semantic Checks for OpenMP Allocate Clause
ClosedPublic

Authored by Rin on Aug 17 2020, 2:13 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
kiranchandramohan requested changes to this revision.Aug 17 2020, 6:09 AM

You missed formatting with clang-format.
Please add additional tests to ensure allocate clause is accepted in all directives where it is allowed.
More comments inline.

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

There are two minor issues with putting this check here,

  1. I think the location of the error should be the allocate clause and not the location of the occurrence of the variable inside the region.
  2. If the variable does not occur inside the parallel region then I think it is still an error as per the standard. I believe the current implementation will miss this case.
917

This should be changed to something like "must appear in a private ...".

This revision now requires changes to proceed.Aug 17 2020, 6:09 AM
flang/lib/Semantics/resolve-directives.cpp
94

Check whether this kind of clause is present in OpenACC also. If not then move it to the OmpDirectiveVisitor class.

104

Is this field required? Can we use an additional check (whether symbol is private) on the dataSharingAttributeObjects_ to obtain this set?

Rin updated this revision to Diff 286568.Aug 19 2020, 8:29 AM

Update Allocate Clause

Rin marked 3 inline comments as done.Aug 19 2020, 8:37 AM
Rin added inline comments.
flang/lib/Semantics/resolve-directives.cpp
104

I should clear this set after each directive. I'll add this in the next update

917

Here it was meant to be "private data-sharing attribute clause". I'll change that in the next update. I'll fix formatting issue in that update also

Rin updated this revision to Diff 286741.Aug 20 2020, 1:43 AM

Updates to Semantic Check

Rin updated this revision to Diff 286746.Aug 20 2020, 2:06 AM

Remove unnecessary set

Rin marked an inline comment as done.Aug 20 2020, 2:07 AM

Thanks Rin for making the changes. A few more comments.

flang/lib/Semantics/resolve-directives.cpp
230–231

Do all OpenMPBlockConstructs have allocate clause?
If not should we restrict this check to only those OpenMPBlockConstructs which have allocate clause?

296

Is a line required above?

337–339

Can this check be done before the find on dataSharingAttributeObjects?

flang/test/Semantics/omp-resolve06.f90
18

We need more error tests.
Use a mix of firstprivate, lastprivate + allocate and private with same and different variables.

Rin updated this revision to Diff 287887.Aug 26 2020, 2:39 AM

Additional tests and fixed semantic checks.

Rin marked 4 inline comments as done.Aug 26 2020, 2:40 AM
Rin updated this revision to Diff 287902.Aug 26 2020, 3:28 AM

Minor Changes

Rin added inline comments.Aug 26 2020, 3:29 AM
flang/lib/Semantics/resolve-directives.cpp
730

This will check for a data sharing attribute object, not a private data sharing attribute object. I will fix this.

Rin updated this revision to Diff 287937.Aug 26 2020, 5:12 AM

Semantic Check fix

flang/lib/Semantics/resolve-directives.cpp
670–771

Is the code in each of the above cases the same? Can we avoid the duplication by the following or making the common code a function?

case llvm::omp::Directive::OMPD_parallel:
case llvm::omp::Directive::OMPD_target:
case llvm::omp::Directive::OMPD_task:
case llvm::omp::Directive::OMPD_teams:
New Code
break;

Rin updated this revision to Diff 288224.Aug 27 2020, 1:10 AM

Additional test and minor updates

Rin marked an inline comment as done.Aug 27 2020, 1:10 AM
Rin updated this revision to Diff 288331.Aug 27 2020, 7:45 AM

Remove extra line

Rin added a reviewer: tskeith.Aug 28 2020, 6:12 AM
clementval added inline comments.Aug 28 2020, 8:35 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
211 ↗(On Diff #288331)

This information is not used in Flang at the moment. Looks like couple of Clang OpenMP tests are failing with this patch and Clang is using this information. So I'm not sure it is worth adding this now or you would have to check the failure in Clang.

Rin added inline comments.Aug 28 2020, 8:39 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
211 ↗(On Diff #288331)

I didn't notice that. I'll make sure to remove this.

Rin updated this revision to Diff 288635.Aug 28 2020, 9:26 AM

Remove OpenMP 5.0 Version Check for Allocate Clause

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

Can you use a range based for loop or std::for_each for this loop and the one below?

685

Can you use std::find_if or range based for.

688

Can break here.

Rin updated this revision to Diff 289129.Sep 1 2020, 4:11 AM

Minor Updates

Rin marked 3 inline comments as done.Sep 1 2020, 4:12 AM
Rin updated this revision to Diff 289660.Sep 3 2020, 1:47 AM

Change set of allocate symbols to set of allocate names

Looking good. Few minor comments. Please wait for one more approval.

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

Thinking about this again, this set will probably not work since different occurrences of names have different pointers. And we probably want to flag error at each occurrence of the same variable. So we can very well have a vector/smallvector. Add a test

  integer ::  N = 2
  integer :: x

  !ERROR: The ALLOCATE clause requires that 'x' must be listed in a private data-sharing attribute clause on the same directive
  !$omp parallel allocate(omp_default_mem_space : x) allocate(omp_default_mem_space : x)
  do i = 1, N
     x = 2
  enddo
  !$omp end parallel

end
679

Check the clang-tidy warning here.

683

I think the ToString() is not required here.
if (allocName->source == symbolPrivate.name())

This revision is now accepted and ready to land.Sep 4 2020, 12:22 AM
Rin updated this revision to Diff 289911.Sep 4 2020, 3:49 AM

Change set of names to vector of names

Rin updated this revision to Diff 289912.Sep 4 2020, 4:05 AM
Rin marked 3 inline comments as done.

Remove unnecessary print statement

Rin added a comment.Sep 8 2020, 8:45 AM

Could someone outside of ARM review this patch as well please?

clementval accepted this revision.Sep 8 2020, 8:54 AM

LGTM (just a small comment about an include)

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

Is this still needed?

Rin added inline comments.Sep 8 2020, 9:01 AM
flang/lib/Semantics/resolve-directives.cpp
21

No, it's not. I'll take it out before submitting the patch upstream.

kiranktp accepted this revision.Sep 8 2020, 9:08 AM

LGTM

Rin updated this revision to Diff 290508.Sep 8 2020, 9:31 AM

Remove unnecessary include

Rin marked an inline comment as done.Sep 8 2020, 9:32 AM
raghavendhra accepted this revision.Sep 8 2020, 10:15 AM
raghavendhra added a subscriber: raghavendhra.

LGTM

This revision was automatically updated to reflect the committed changes.