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.
Details
- Reviewers
kkwli0 arpith-jacob carlo.bertolli ABataev hfinkel - Commits
- rGa9f35cb7d665: [OpenMP] Add support for multidimensional array sections in map clause SEMA.
rC263019: [OpenMP] Add support for multidimensional array sections in map clause SEMA.
rL263019: [OpenMP] Add support for multidimensional array sections in map clause SEMA.
Diff Detail
Event Timeline
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() |
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! |
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. |
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. |
- 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. |
lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
9127 | This must return 'bool', not 'int' |
lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
9127 | Sorry, my bad. It is fixed in the new diff. |
If you say 'can be' incompatible, then this must be a warning, not an error