This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Add support for multidimensional array sections in map clause SEMA.
ClosedPublic

Authored by sfantao on Feb 23 2016, 10:03 AM.

Details

Summary

In some cases it can be proved statically that multidimensional array section refer to contiguous storage and can therefore be allowed in a map clause. This patch adds support for those cases in SEMA.

Diff Detail

Event Timeline

sfantao updated this revision to Diff 48827.Feb 23 2016, 10:03 AM
sfantao retitled this revision from to [OpenMP] Add support for multidimensional array sections in map clause SEMA..
sfantao updated this object.
ABataev added inline comments.Feb 25 2016, 7:30 PM
include/clang/Basic/DiagnosticSemaKinds.td
7812–7814

If you say 'can be' incompatible, then this must be a warning, not an error

lib/Sema/SemaOpenMP.cpp
9077–9084

I can't agree with that. For example:

const int n = 0;
arr[n:]

It won't work with your solution, though we shall support it

9298–9310

OMPArraySectionExpr has static function getBaseOriginalType()

sfantao updated this revision to Diff 49239.Feb 26 2016, 2:19 PM
sfantao marked 3 inline comments as done.

Use better disgnostic message and OMPArraySectionExpr::getBaseOriginalType.

Hi Alexey,

Thanks for the review!

include/clang/Basic/DiagnosticSemaKinds.td
7812–7814

I agree the diagnostic description is not the best. I changed it to: "can't prove employed array section specifies contiguous storage".

This has to be an error because there is no way to express non-contiguous in the offload runtime interface. So, we would have to codegen traps. The user would end up getting runtime crashes instead of a diagnostic that would be far more useful to direct the user to take the right actions.

lib/Sema/SemaOpenMP.cpp
9077–9084

Hi Alexey,

That example works fine. Note that CheckArrayExpressionReferToWholeSize only results in a error if AllowWholeSizeArraySection = false. It is initially set to true, and will be set to false as components of the expression prove to be incompatible with that. I added a few regression test for when the bounds come from variables.

So:

struct S1 {
  int a;
  int b;
}
struct S2 {
  S1 a[10];
  int b;
}

foo (int arg) {
  int a[5][6];
  const int n = 0;
  S2 s;

  // valid - the array expression is in the right most component.
  #pragma omp target map(a[arg:])
  #pragma omp target map(a[:arg])
  #pragma omp target map(a[n:])
  // valid - this is valid only if n is zero and the compiler can prove that.
  #pragma omp target map(a[:][n:])

  // invalid - is only valid if arg is zero and that cannot be proved.
  #pragma omp target map(a[:][arg:])

  // invalid - it is contiguous storage but the OpenMP 4.5 spec explicitly say array sections only allowed in the rightmost expression if struct fields are involved.
  #pragma omp target map(s.a[n:1].b)
}
9298–9310

Oh, great, I hadn't noticed. I am using that now. Thanks!

ABataev added inline comments.Mar 2 2016, 2:24 AM
include/clang/Basic/DiagnosticSemaKinds.td
7812–7814

'can't prove' again is not good for an error. Still think this must be a warning or you don't need to diagnose anything if you can't make a decision. In this case, you must consider specified array section as a contiguous.

lib/Sema/SemaOpenMP.cpp
9077–9084

I agree with all your examples except for this one

#pragma omp target map(a[:][arg:])

For me, this is valid if you can't prove it is non-contiguous. You should consider this as a possibly contiguous. If it is not contiguous, it is user's problem. But if he needs to use such form of expression, I don't see why we should not allow him to do this.

ABataev added inline comments.Mar 2 2016, 2:26 AM
lib/Sema/SemaOpenMP.cpp
9077–9084

I don't think this code

const int n = 0;
arr[n:]

will work in C. Try to add a test for C, not C++. In C 'const' does not mean you can evaluate the value of the variable during compilation.

sfantao updated this revision to Diff 49709.Mar 2 2016, 9:53 PM
sfantao marked 3 inline comments as done.
  • Allow array expressions whose memory contiguous checks are inconclusive.
  • Add C test to check the constant expressions no longer can prove bounds in array sections.
include/clang/Basic/DiagnosticSemaKinds.td
7812–7814

Now I am only emitting an error for the cases that can be proved wrong. For those that are inconclusive I don't issue any warning. The message also changed to reflect that.

lib/Sema/SemaOpenMP.cpp
9077–9084

In the new diff I am allowing this given that the ranges check is inconclusive. At runtime the behavior will be the same as map(a[:]), so the runtime map will contain the holes. This falls into those cases that is users' fault.

9077–9084

So if you have:

const int n = 0;
arr[:][n:]

This is not a probably anymore because the check in the second dimension will be inconclusive, so it will be allowed even if n can't be proved to be zero.

I'm adding a test anyways.

ABataev added inline comments.Mar 2 2016, 10:58 PM
lib/Sema/SemaOpenMP.cpp
9074

Don't use 'int' as a result of the functions, add enumeric and use it.

9103–9105

Remove extra braces

9130

Again, use enum as a return result

9290–9292

Extra braces

sfantao updated this revision to Diff 49749.Mar 3 2016, 9:55 AM
sfantao marked 4 inline comments as done.
  • Remove extra braces and revert the sense of the whole/unity array section check.
lib/Sema/SemaOpenMP.cpp
9074

I changed this back to bool and reverted the sense given that I am only taking any action if something is proved NOT to be whole or unity.

9130

Using bool instead with reverted sense.

ABataev added inline comments.Mar 3 2016, 8:45 PM
lib/Sema/SemaOpenMP.cpp
9127

This must return 'bool', not 'int'

sfantao updated this revision to Diff 49827.Mar 4 2016, 7:30 AM
  • Change return value from int to bool.
sfantao marked an inline comment as done.Mar 4 2016, 7:32 AM
sfantao added inline comments.
lib/Sema/SemaOpenMP.cpp
9127

Sorry, my bad. It is fixed in the new diff.

ABataev accepted this revision.Mar 8 2016, 9:31 PM
ABataev edited edge metadata.

LG

This revision is now accepted and ready to land.Mar 8 2016, 9:31 PM
sfantao closed this revision.Mar 9 2016, 7:50 AM
sfantao marked an inline comment as done.