sfantao added a comment. Hi Alexey,
Thanks for the review! ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7767-7768 @@ -7766,4 +7766,4 @@ "bit fields cannot be used to specify storage in a map clause">; -def err_omp_array_section_in_rightmost_expression : Error< - "array section can only be associated with the rightmost variable in a map clause expression">; +def err_omp_array_section_leads_to_non_contiguous_storage : Error< + "employed array section is or can be incompatible with contiguous storage requirements">; def err_omp_union_type_not_allowed : Error< ---------------- ABataev wrote: > If you say 'can be' incompatible, then this must be a warning, not an error 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. ================ Comment at: lib/Sema/SemaOpenMP.cpp:8988-8995 @@ +8987,10 @@ + + // If this is an array subscript, it refers to the whole size if the size of + // the dimension is constant and equals 1. Also, an array section assumes the + // format of an array subscript if no colon is used. + if (isa<ArraySubscriptExpr>(E) || (OASE && OASE->getColonLoc().isInvalid())) { + if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr())) + return ATy->getSize().getSExtValue() == 1; + return false; + } + ---------------- ABataev wrote: > 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 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) } ``` ================ Comment at: lib/Sema/SemaOpenMP.cpp:9208-9220 @@ -9113,6 +9207,15 @@ + // Determine the dimension we care about. We need to skip all the nested + // array sections to determine that. + unsigned Dimension = 0; + auto *BaseE = E; + while (auto *SE = dyn_cast<OMPArraySectionExpr>(BaseE)) { + BaseE = SE->getBase()->IgnoreParenImpCasts(); + ++Dimension; + } + // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, C++, p.1] // If the type of a list item is a reference to a type T then the type // will be considered to be T for all purposes of this clause. - QualType CurType = E->getType(); + QualType CurType = BaseE->getType(); if (CurType->isReferenceType()) ---------------- ABataev wrote: > OMPArraySectionExpr has static function getBaseOriginalType() Oh, great, I hadn't noticed. I am using that now. Thanks! http://reviews.llvm.org/D17547 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits