<div class="socmaildefaultfont" dir="ltr" style="font-family:Arial;font-size:10.5pt" ><div dir="ltr" >Hi Richard,</div>
<div dir="ltr" > </div>
<div dir="ltr" >Thanks for catching that! Fixed in r275930.</div>
<div dir="ltr" > </div>
<div dir="ltr" >Thanks again,</div>
<div dir="ltr" >Samuel</div>
<div dir="ltr" > </div>
<blockquote data-history-content-modified="1" dir="ltr" style="border-left:solid #aaaaaa 2px; margin-left:5px; padding-left:5px; direction:ltr; margin-right:0px" >----- Original message -----<br>From: Richard Smith <richard@metafoo.co.uk><br>Sent by: metafoo@gmail.com<br>To: Samuel F Antao/Watson/IBM@IBMUS<br>Cc: cfe-commits <cfe-commits@lists.llvm.org><br>Subject: Re: r263019 - [OpenMP] Add support for multidimensional array sections in map clause SEMA.<br>Date: Mon, Jul 18, 2016 6:03 PM<br>
<div dir="ltr" ><div><div>On Wed, Mar 9, 2016 at 7:46 AM, Samuel Antao via cfe-commits <span dir="ltr" ><<a href="mailto:cfe-commits@lists.llvm.org" target="_blank" >cfe-commits@lists.llvm.org</a>></span> wrote:
<blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" >Author: sfantao<br>Date: Wed Mar 9 09:46:05 2016<br>New Revision: 263019<br><br>URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263019&view=rev" rel="noreferrer" target="_blank" >http://llvm.org/viewvc/llvm-project?rev=263019&view=rev</a><br>Log:<br>[OpenMP] Add support for multidimensional array sections in map clause SEMA.<br><br>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.<br><br>Reviewers: hfinkel, carlo.bertolli, arpith-jacob, kkwli0, ABataev<br><br>Subscribers: cfe-commits, fraggamuffin, caomhin<br><br>Differential Revision: <a href="http://reviews.llvm.org/D17547" rel="noreferrer" target="_blank" >http://reviews.llvm.org/D17547</a><br><br>Modified:<br> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br> cfe/trunk/lib/Sema/SemaOpenMP.cpp<br> cfe/trunk/test/OpenMP/target_map_messages.cpp<br><br>Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=263019&r1=263018&r2=263019&view=diff" rel="noreferrer" target="_blank" >http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=263019&r1=263018&r2=263019&view=diff</a><br>==============================================================================<br>--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Wed Mar 9 09:46:05 2016<br>@@ -7825,8 +7825,8 @@ def err_omp_expected_named_var_member_or<br> "expected expression containing only member accesses and/or array sections based on named variables">;<br> def err_omp_bit_fields_forbidden_in_map_clause : Error<<br> "bit fields cannot be used to specify storage in a map clause">;<br>-def err_omp_array_section_in_rightmost_expression : Error<<br>- "array section can only be associated with the rightmost variable in a map clause expression">;<br>+def err_array_section_does_not_specify_contiguous_storage : Error<<br>+ "array section does not specify contiguous storage">;<br> def err_omp_union_type_not_allowed : Error<<br> "mapped storage cannot be derived from a union">;<br> def err_omp_expected_access_to_data_field : Error<<br><br>Modified: cfe/trunk/lib/Sema/SemaOpenMP.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOpenMP.cpp?rev=263019&r1=263018&r2=263019&view=diff" rel="noreferrer" target="_blank" >http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaOpenMP.cpp?rev=263019&r1=263018&r2=263019&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/Sema/SemaOpenMP.cpp (original)<br>+++ cfe/trunk/lib/Sema/SemaOpenMP.cpp Wed Mar 9 09:46:05 2016<br>@@ -9104,6 +9104,94 @@ static bool CheckTypeMappable(SourceLoca<br> return true;<br> }<br><br>+/// \brief Return true if it can be proven that the provided array expression<br>+/// (array section or array subscript) does NOT specify the whole size of the<br>+/// array whose base type is \a BaseQTy.<br>+static bool CheckArrayExpressionDoesNotReferToWholeSize(Sema &SemaRef,<br>+ const Expr *E,<br>+ QualType BaseQTy) {<br>+ auto *OASE = dyn_cast<OMPArraySectionExpr>(E);<br>+<br>+ // If this is an array subscript, it refers to the whole size if the size of<br>+ // the dimension is constant and equals 1. Also, an array section assumes the<br>+ // format of an array subscript if no colon is used.<br>+ if (isa<ArraySubscriptExpr>(E) || (OASE && OASE->getColonLoc().isInvalid())) {<br>+ if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr()))<br>+ return ATy->getSize().getSExtValue() != 1;<br>+ // Size can't be evaluated statically.<br>+ return false;<br>+ }<br>+<br>+ assert(OASE && "Expecting array section if not an array subscript.");<br>+ auto *LowerBound = OASE->getLowerBound();<br>+ auto *Length = OASE->getLength();<br>+<br>+ // If there is a lower bound that does not evaluates to zero, we are not<br>+ // convering the whole dimension.<br>+ if (LowerBound) {<br>+ llvm::APSInt ConstLowerBound;<br>+ if (!LowerBound->EvaluateAsInt(ConstLowerBound, SemaRef.getASTContext()))<br>+ return false; // Can't get the integer value as a constant.<br>+ if (ConstLowerBound.getSExtValue())<br>+ return true;<br>+ }<br>+<br>+ // If we don't have a length we covering the whole dimension.<br>+ if (!Length)<br>+ return false;<br>+<br>+ // If the base is a pointer, we don't have a way to get the size of the<br>+ // pointee.<br>+ if (BaseQTy->isPointerType())<br>+ return false;<br>+<br>+ // We can only check if the length is the same as the size of the dimension<br>+ // if we have a constant array.<br>+ auto *CATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr());<br>+ if (!CATy)<br>+ return false;<br>+<br>+ llvm::APSInt ConstLength;<br>+ if (!Length->EvaluateAsInt(ConstLength, SemaRef.getASTContext()))<br>+ return false; // Can't get the integer value as a constant.<br>+<br>+ return CATy->getSize().getSExtValue() != ConstLength.getSExtValue();<br>+}<br>+<br>+// Return true if it can be proven that the provided array expression (array<br>+// section or array subscript) does NOT specify a single element of the array<br>+// whose base type is \a BaseQTy.<br>+static bool CheckArrayExpressionDoesNotReferToUnitySize(Sema &SemaRef,<br>+ const Expr *E,<br>+ QualType BaseQTy) {<br>+ auto *OASE = dyn_cast<OMPArraySectionExpr>(E);<br>+<br>+ // An array subscript always refer to a single element. Also, an array section<br>+ // assumes the format of an array subscript if no colon is used.<br>+ if (isa<ArraySubscriptExpr>(E) || (OASE && OASE->getColonLoc().isInvalid()))<br>+ return false;<br>+<br>+ assert(OASE && "Expecting array section if not an array subscript.");<br>+ auto *Length = OASE->getLength();<br>+<br>+ // If we don't have a length we have to check if the array has unitary size<br>+ // for this dimension. Also, we should always expect a length if the base type<br>+ // is pointer.<br>+ if (!Length) {<br>+ if (auto *ATy = dyn_cast<ConstantArrayType>(BaseQTy.getTypePtr()))<br>+ return ATy->getSize().getSExtValue() != 1;<br>+ // We cannot assume anything.<br>+ return false;<br>+ }<br>+<br>+ // Check if the length evaluates to 1.<br>+ llvm::APSInt ConstLength;<br>+ if (!Length->EvaluateAsInt(ConstLength, SemaRef.getASTContext()))<br>+ return false; // Can't get the integer value as a constant.<br>+<br>+ return ConstLength.getSExtValue() != 1;<br>+}<br>+<br> // Return the expression of the base of the map clause or null if it cannot<br> // be determined and do all the necessary checks to see if the expression is<br> // valid as a standalone map clause expression.<br>@@ -9132,14 +9220,13 @@ static Expr *CheckMapClauseExpressionBas<br><br> Expr *RelevantExpr = nullptr;<br><br>- // Flags to help capture some memory<br>-<br> // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, p.2]<br> // If a list item is an array section, it must specify contiguous storage.<br> //<br> // For this restriction it is sufficient that we make sure only references<br> // to variables or fields and array expressions, and that no array sections<br>- // exist except in the rightmost expression. E.g. these would be invalid:<br>+ // exist except in the rightmost expression (unless they cover the whole<br>+ // dimension of the array). E.g. these would be invalid:<br> //<br> // r.ArrS[3:5].Arr[6:7]<br> //<br>@@ -9150,12 +9237,11 @@ static Expr *CheckMapClauseExpressionBas<br> //<br> // r.ArrS[3].x<br><br>- bool IsRightMostExpression = true;<br>-<br>- while (!RelevantExpr) {<br>- auto AllowArraySection = IsRightMostExpression;<br>- IsRightMostExpression = false;<br>+ bool AllowUnitySizeArraySection = true;<br>+ bool AllowWholeSizeArraySection = true;<br><br>+ for (bool IsRightMostExpression = true; !RelevantExpr;<br>+ IsRightMostExpression = false) {<br> E = E->IgnoreParenImpCasts();<br><br> if (auto *CurE = dyn_cast<DeclRefExpr>(E)) {<br>@@ -9163,6 +9249,11 @@ static Expr *CheckMapClauseExpressionBas<br> break;<br><br> RelevantExpr = CurE;<br>+<br>+ // If we got a reference to a declaration, we should not expect any array<br>+ // section before that.<br>+ AllowUnitySizeArraySection = false;<br>+ AllowWholeSizeArraySection = false;<br> continue;<br> }<br><br>@@ -9195,9 +9286,7 @@ static Expr *CheckMapClauseExpressionBas<br> // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, C++, p.1]<br> // If the type of a list item is a reference to a type T then the type<br> // will be considered to be T for all purposes of this clause.<br>- QualType CurType = BaseE->getType();<br>- if (CurType->isReferenceType())<br>- CurType = CurType->getPointeeType();<br>+ QualType CurType = BaseE->getType().getNonReferenceType();<br><br> // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, C/C++, p.2]<br> // A list item cannot be a variable that is a member of a structure with<br>@@ -9210,6 +9299,15 @@ static Expr *CheckMapClauseExpressionBas<br> break;<br> }<br><br>+ // If we got a member expression, we should not expect any array section<br>+ // before that:<br>+ //<br>+ // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, p.7]<br>+ // If a list item is an element of a structure, only the rightmost symbol<br>+ // of the variable reference can be an array section.<br>+ //<br>+ AllowUnitySizeArraySection = false;<br>+ AllowWholeSizeArraySection = false;<br> continue;<br> }<br><br>@@ -9221,35 +9319,58 @@ static Expr *CheckMapClauseExpressionBas<br> << 0 << CurE->getSourceRange();<br> break;<br> }<br>+<br>+ // If we got an array subscript that express the whole dimension we<br>+ // can have any array expressions before. If it only expressing part of<br>+ // the dimension, we can only have unitary-size array expressions.<br>+ if (CheckArrayExpressionDoesNotReferToWholeSize(SemaRef, CurE,<br>+ E->getType()))<br>+ AllowWholeSizeArraySection = false;<br> continue;<br> }<br><br> if (auto *CurE = dyn_cast<OMPArraySectionExpr>(E)) {<br>- // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, p.7]<br>- // If a list item is an element of a structure, only the rightmost symbol<br>- // of the variable reference can be an array section.<br>- //<br>- if (!AllowArraySection) {<br>- SemaRef.Diag(ELoc, diag::err_omp_array_section_in_rightmost_expression)<br>- << CurE->getSourceRange();<br>- break;<br>- }<br>-<br> E = CurE->getBase()->IgnoreParenImpCasts();<br><br>+ auto CurType =<br>+ OMPArraySectionExpr::getBaseOriginalType(E).getCanonicalType();<br>+<br> // OpenMP 4.5 [2.15.5.1, map Clause, Restrictions, C++, p.1]<br> // If the type of a list item is a reference to a type T then the type<br> // will be considered to be T for all purposes of this clause.<br>- QualType CurType = E->getType();<br> if (CurType->isReferenceType())<br> CurType = CurType->getPointeeType();<br><br>- if (!CurType->isAnyPointerType() && !CurType->isArrayType()) {<br>+ bool IsPointer = CurType->isAnyPointerType();<br>+<br>+ if (!IsPointer && !CurType->isArrayType()) {<br> SemaRef.Diag(ELoc, diag::err_omp_expected_base_var_name)<br> << 0 << CurE->getSourceRange();<br> break;<br> }<br><br>+ bool NotWhole =<br>+ CheckArrayExpressionDoesNotReferToWholeSize(SemaRef, CurE, CurType);<br>+ bool NotUnity =<br>+ CheckArrayExpressionDoesNotReferToUnitySize(SemaRef, CurE, CurType);<br>+<br>+ if (AllowWholeSizeArraySection && AllowUnitySizeArraySection) {<br>+ // Any array section is currently allowed.<br>+ //<br>+ // If this array section refers to the whole dimension we can still<br>+ // accept other array sections before this one, except if the base is a<br>+ // pointer. Otherwise, only unitary sections are accepted.<br>+ if (NotWhole || IsPointer)<br>+ AllowWholeSizeArraySection = false;<br>+ } else if ((AllowUnitySizeArraySection && NotUnity) ||<br>+ (AllowWholeSizeArraySection && NotWhole)) {</blockquote>
<div> </div>
<div>[Found by Coverity]</div>
<div> </div>
<div>You have dead code here. Note that AllowUnitySizeArraySection is true whenever AllowWholeSizeArraySection is true. Therefore whenever AllowWholeSizeArraySection is true, we enter the first branch of this 'if', so the whole (AllowWholeSizeArraySection && NotWhole) calculation is only ever reached with AllowWholeSizeArraySection == false.</div>
<div> </div>
<blockquote style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex" >+ // A unity or whole array section is not allowed and that is not<br>+ // compatible with the properties of the current array section.<br>+ SemaRef.Diag(<br>+ ELoc, diag::err_array_section_does_not_specify_contiguous_storage)<br>+ << CurE->getSourceRange();<br>+ break;<br>+ }<br> continue;<br> }<br><br><br>Modified: cfe/trunk/test/OpenMP/target_map_messages.cpp<br>URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/target_map_messages.cpp?rev=263019&r1=263018&r2=263019&view=diff" rel="noreferrer" target="_blank" >http://llvm.org/viewvc/llvm-project/cfe/trunk/test/OpenMP/target_map_messages.cpp?rev=263019&r1=263018&r2=263019&view=diff</a><br>==============================================================================<br>--- cfe/trunk/test/OpenMP/target_map_messages.cpp (original)<br>+++ cfe/trunk/test/OpenMP/target_map_messages.cpp Wed Mar 9 09:46:05 2016<br>@@ -1,4 +1,21 @@<br>-// RUN: %clang_cc1 -verify -fopenmp -ferror-limit 100 %s<br>+// RUN: %clang_cc1 -verify -fopenmp -ferror-limit 200 %s<br>+// RUN: %clang_cc1 -DCCODE -verify -fopenmp -ferror-limit 200 -x c %s<br>+#ifdef CCODE<br>+void foo(int arg) {<br>+ const int n = 0;<br>+<br>+ double marr[10][10][10];<br>+<br>+ #pragma omp target map(marr[2][0:2][0:2]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:][0:][:])<br>+ {}<br>+ #pragma omp target map(marr[:][1:][:]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:][n:][:])<br>+ {}<br>+}<br>+#else<br> template <typename T, int I><br> struct SA {<br> static int ss;<br>@@ -82,6 +99,13 @@ union SD {<br> void SAclient(int arg) {<br> SA<int,123> s;<br> s.func(arg); // expected-note {{in instantiation of member function}}<br>+ double marr[10][10][10];<br>+ double marr2[5][10][1];<br>+ double mvla[5][arg][10];<br>+ double ***mptr;<br>+ const int n = 0;<br>+ const int m = 1;<br>+ double mvla2[5][arg][m+n+10];<br><br> SB *p;<br><br>@@ -89,10 +113,106 @@ void SAclient(int arg) {<br> SC r(p),t(p);<br> #pragma omp target map(r)<br> {}<br>+ #pragma omp target map(marr[2][0:2][0:2]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:][0:2][0:2]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[2][3][0:2])<br>+ {}<br>+ #pragma omp target map(marr[:][:][:])<br>+ {}<br>+ #pragma omp target map(marr[:2][:][:])<br>+ {}<br>+ #pragma omp target map(marr[arg:][:][:])<br>+ {}<br>+ #pragma omp target map(marr[arg:])<br>+ {}<br>+ #pragma omp target map(marr[arg:][:arg][:]) // correct if arg is the size of dimension 2<br>+ {}<br>+ #pragma omp target map(marr[:arg][:])<br>+ {}<br>+ #pragma omp target map(marr[:arg][n:])<br>+ {}<br>+ #pragma omp target map(marr[:][:arg][n:]) // correct if arg is the size of dimension 2<br>+ {}<br>+ #pragma omp target map(marr[:][:m][n:]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[n:m][:arg][n:])<br>+ {}<br>+ #pragma omp target map(marr[:2][:1][:]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:2][1:][:]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:2][:][:1]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:2][:][1:]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:1][:2][:])<br>+ {}<br>+ #pragma omp target map(marr[:1][0][:])<br>+ {}<br>+ #pragma omp target map(marr[:arg][:2][:]) // correct if arg is 1<br>+ {}<br>+ #pragma omp target map(marr[:1][3:1][:2])<br>+ {}<br>+ #pragma omp target map(marr[:1][3:arg][:2]) // correct if arg is 1<br>+ {}<br>+ #pragma omp target map(marr[:1][3:2][:2]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr[:2][:10][:])<br>+ {}<br>+ #pragma omp target map(marr[:2][:][:5+5])<br>+ {}<br>+ #pragma omp target map(marr[:2][2+2-4:][0:5+5])<br>+ {}<br>+<br>+ #pragma omp target map(marr[:1][:2][0]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(marr2[:1][:2][0])<br>+ {}<br>+<br>+ #pragma omp target map(mvla[:1][:][0]) // correct if the size of dimension 2 is 1.<br>+ {}<br>+ #pragma omp target map(mvla[:2][:arg][:]) // correct if arg is the size of dimension 2.<br>+ {}<br>+ #pragma omp target map(mvla[:1][:2][0]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(mvla[1][2:arg][:])<br>+ {}<br>+ #pragma omp target map(mvla[:1][:][:])<br>+ {}<br>+ #pragma omp target map(mvla2[:1][:2][:11])<br>+ {}<br>+ #pragma omp target map(mvla2[:1][:2][:10]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+<br>+ #pragma omp target map(mptr[:2][2+2-4:1][0:5+5]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(mptr[:1][:2-1][2:4-3])<br>+ {}<br>+ #pragma omp target map(mptr[:1][:arg][2:4-3]) // correct if arg is 1.<br>+ {}<br>+ #pragma omp target map(mptr[:1][:2-1][0:2])<br>+ {}<br>+ #pragma omp target map(mptr[:1][:2][0:2]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+ #pragma omp target map(mptr[:1][:][0:2]) // expected-error {{section length is unspecified and cannot be inferred because subscripted value is not an array}}<br>+ {}<br>+ #pragma omp target map(mptr[:2][:1][0:2]) // expected-error {{array section does not specify contiguous storage}}<br>+ {}<br>+<br> #pragma omp target map(r.ArrS[0].B)<br> {}<br>+ #pragma omp target map(r.ArrS[:1].B) // expected-error {{OpenMP array section is not allowed here}}<br>+ {}<br>+ #pragma omp target map(r.ArrS[:arg].B) // expected-error {{OpenMP array section is not allowed here}}<br>+ {}<br> #pragma omp target map(r.ArrS[0].Arr[1:23])<br> {}<br>+ #pragma omp target map(r.ArrS[0].Arr[1:arg])<br>+ {}<br>+ #pragma omp target map(r.ArrS[0].Arr[arg:23])<br>+ {}<br> #pragma omp target map(r.ArrS[0].Error) // expected-error {{no member named 'Error' in 'SB'}}<br> {}<br> #pragma omp target map(r.ArrS[0].A, r.ArrS[1].A) // expected-error {{multiple array elements associated with the same variable are not allowed in map clauses of the same construct}} expected-note {{used here}}<br>@@ -382,4 +502,4 @@ int main(int argc, char **argv) {<br> foo();<br> return tmain<int, 3>(argc)+tmain<from, 4>(argc); // expected-note {{in instantiation of function template specialization 'tmain<int, 3>' requested here}} expected-note {{in instantiation of function template specialization 'tmain<int, 4>' requested here}}<br> }<br>-<br>+#endif<br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@lists.llvm.org" target="_blank" >cfe-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank" >http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a></blockquote></div></div></div></blockquote>
<div dir="ltr" > </div></div><BR>