r228382 - OpenCL: handle shift operator with vector operands
Pekka Jääskeläinen
pekka.jaaskelainen at tut.fi
Tue Feb 10 08:43:27 PST 2015
Hello,
I'm not knowledgeable enough on the Clang internals either (still
learning). Anyways, should this test that code is generated
correctly for <<=, not just that Sema doesn't complain about it?
Other than this LGTM.
Pekka
On 02/10/2015 05:54 PM, Sahasrabuddhe, Sameer wrote:
> Hello Jeroen,
>
> Thanks for the feedback. Now waiting for an LGTM from Pekka.
>
> Sameer.
> ------------------------------------------------------------------------------
> *From:* Jeroen Ketema [j.ketema at imperial.ac.uk]
> *Sent:* Tuesday, February 10, 2015 5:25 PM
> *To:* Sahasrabuddhe, Sameer
> *Cc:* llvm cfe; Tom Stellard; Pekka Jääskeläinen
> *Subject:* Re: r228382 - OpenCL: handle shift operator with vector operands
>
>
> Hi,
>
> I’m not knowledgeable enough to review the updated patch, but all my test
> cases now pass again.
>
> Jeroen
>
>> On 09 Feb 2015, at 11:00, Sameer Sahasrabuddhe <sameer.sahasrabuddhe at amd.com
>> <mailto:sameer.sahasrabuddhe at amd.com>> wrote:
>>
>> Hi all,
>>
>> Attached an updated patch that fixes the earlier patch for vector shift
>> operators in OpenCL. Note the new argument "IsCompAssign" for function
>> checkOpenCLVectorShift(), which was not handled in earlier patch.
>>
>> Sameer.
>>
>> On 2/6/2015 11:02 PM, Tom Stellard wrote:
>>> On Fri, Feb 06, 2015 at 04:05:22PM +0000, Sahasrabuddhe, Sameer wrote:
>>>> Hello Jeroen,
>>>>
>>>> Thanks for pointing this out! I will fix this as soon as possible, but in the meantime, request you or anyone with commit access to please revert my commit! I do not have access to my work computer until Monday morning India time. (Reminder to self: *never* commit on a Friday!).
>>> Hi,
>>>
>>> I have reverted this. Can you add this test case when you recommit.
>>>
>>> Thanks,
>>> Tom
>>>
>>>> Sameer.
>>>> ________________________________________
>>>> From: Jeroen Ketema [j.ketema at imperial.ac.uk]
>>>> Sent: Friday, February 06, 2015 9:15 PM
>>>> To: Sahasrabuddhe, Sameer
>>>> Cc: llvm cfe
>>>> Subject: Re: r228382 - OpenCL: handle shift operator with vector operands
>>>>
>>>> Hi Sameer,
>>>>
>>>> This commit breaks augmented assignment in combination with shifting.
>>>>
>>>> Consider the following:
>>>>
>>>> typedef __attribute__((ext_vector_type(3))) char char3;
>>>>
>>>> void foo() {
>>>> char3 v = {1,1,1};
>>>> char3 w = {1,2,3};
>>>>
>>>> w <<= v;
>>>> }
>>>>
>>>> If I compile with:
>>>>
>>>> clang -x cl file.c
>>>>
>>>> Then an error is produced:
>>>>
>>>> file.c:10:5: error: expression is not assignable
>>>> w <<= v;
>>>> ~ ^
>>>> 1 error generated.
>>>>
>>>> This while the OpenCL 1.2 spec says that “w <<= v” is short for “w = w << v”,
>>>> and which does not produce an error.
>>>>
>>>> Thanks,
>>>>
>>>> Jeroen
>>>>
>>>>> On 06 Feb 2015, at 05:44, Sameer Sahasrabuddhe <sameer.sahasrabuddhe atamd.com <http://amd.com>> wrote:
>>>>>
>>>>> Author: sameerds
>>>>> Date: Thu Feb 5 23:44:55 2015
>>>>> New Revision: 228382
>>>>>
>>>>> URL:http://llvm.org/viewvc/llvm-project?rev=228382&view=rev
>>>>> Log:
>>>>> OpenCL: handle shift operator with vector operands
>>>>>
>>>>> Introduce a number of checks:
>>>>> 1. If LHS is a scalar, then RHS cannot be a vector.
>>>>> 2. Operands must be of integer type.
>>>>> 3. If both are vectors, then the number of elements must match.
>>>>>
>>>>> Relax the requirement for "usual arithmetic conversions":
>>>>> When LHS is a vector, a scalar RHS can simply be expanded into a
>>>>> vector; OpenCL does not require that its rank be lower than the LHS.
>>>>> For example, the following code is not an error even if the implicit
>>>>> type of the constant literal is "int".
>>>>>
>>>>> char2 foo(char2 v) { return v << 1; }
>>>>>
>>>>> Consolidate existing tests under CodeGenOpenCL, and add more tests
>>>>> under SemaOpenCL.
>>>>>
>>>>>
>>>>> Modified:
>>>>> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>>>> cfe/trunk/lib/Sema/SemaExpr.cpp
>>>>> cfe/trunk/test/CodeGenOpenCL/shifts.cl
>>>>> cfe/trunk/test/SemaOpenCL/shifts.cl
>>>>>
>>>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>>>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=228382&r1=228381&r2=228382&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Feb 5 23:44:55 2015
>>>>> @@ -2031,6 +2031,8 @@ def err_typecheck_vector_not_convertable
>>>>> "can't convert between vector values of different size (%0 and %1)">;
>>>>> def err_typecheck_vector_not_convertable_non_scalar : Error<
>>>>> "can't convert between vector and non-scalar values (%0 and %1)">;
>>>>> +def err_typecheck_vector_lengths_not_equal : Error<
>>>>> + "vector operands do not have the same number of elements (%0 and %1)">;
>>>>> def err_ext_vector_component_exceeds_length : Error<
>>>>> "vector component access exceeds type %0">;
>>>>> def err_ext_vector_component_name_illegal : Error<
>>>>> @@ -4884,6 +4886,8 @@ def err_ivar_reference_type : Error<
>>>>> "instance variables cannot be of reference type">;
>>>>> def err_typecheck_illegal_increment_decrement : Error<
>>>>> "cannot %select{decrement|increment}1 value of type %0">;
>>>>> +def err_typecheck_expect_int : Error<
>>>>> + "used type %0 where integer is required">;
>>>>> def err_typecheck_arithmetic_incomplete_type : Error<
>>>>> "arithmetic on a pointer to an incomplete type %0">;
>>>>> def err_typecheck_pointer_arith_function_type : Error<
>>>>> @@ -5047,6 +5051,9 @@ def warn_null_in_comparison_operation :
>>>>> "comparison between NULL and non-pointer "
>>>>> "%select{(%1 and NULL)|(NULL and %1)}0">,
>>>>> InGroup<NullArithmetic>;
>>>>> +def err_shift_rhs_only_vector : Error<
>>>>> + "requested shift is a vector of type %0 but the first operand is not a "
>>>>> + "vector (%1)">;
>>>>>
>>>>> def warn_logical_not_on_lhs_of_comparison : Warning<
>>>>> "logical not is only applied to the left hand side of this comparison">,
>>>>>
>>>>> Modified: cfe/trunk/lib/Sema/SemaExpr.cpp
>>>>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExpr.cpp?rev=228382&r1=228381&r2=228382&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/lib/Sema/SemaExpr.cpp (original)
>>>>> +++ cfe/trunk/lib/Sema/SemaExpr.cpp Thu Feb 5 23:44:55 2015
>>>>> @@ -7772,6 +7772,67 @@ static void DiagnoseBadShiftValues(Sema&
>>>>> << RHS.get()->getSourceRange();
>>>>> }
>>>>>
>>>>> +/// \brief Return the resulting type when an OpenCL vector is shifted
>>>>> +/// by a scalar or vector shift amount.
>>>>> +static QualType checkOpenCLVectorShift(Sema &S,
>>>>> + ExprResult &LHS, ExprResult &RHS,
>>>>> + SourceLocation Loc) {
>>>>> + // OpenCL v1.1 s6.3.j says RHS can be a vector only if LHS is a vector.
>>>>> + if (!LHS.get()->getType()->isVectorType()) {
>>>>> + S.Diag(Loc, diag::err_shift_rhs_only_vector)
>>>>> + << RHS.get()->getType() << LHS.get()->getType()
>>>>> + << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
>>>>> + return QualType();
>>>>> + }
>>>>> +
>>>>> + LHS = S.UsualUnaryConversions(LHS.get());
>>>>> + if (LHS.isInvalid()) return QualType();
>>>>> +
>>>>> + RHS = S.UsualUnaryConversions(RHS.get());
>>>>> + if (RHS.isInvalid()) return QualType();
>>>>> +
>>>>> + QualType LHSType = LHS.get()->getType();
>>>>> + const VectorType *LHSVecTy = LHSType->getAs<VectorType>();
>>>>> + QualType LHSEleType = LHSVecTy->getElementType();
>>>>> +
>>>>> + // Note that RHS might not be a vector.
>>>>> + QualType RHSType = RHS.get()->getType();
>>>>> + const VectorType *RHSVecTy = RHSType->getAs<VectorType>();
>>>>> + QualType RHSEleType = RHSVecTy ? RHSVecTy->getElementType() : RHSType;
>>>>> +
>>>>> + // OpenCL v1.1 s6.3.j says that the operands need to be integers.
>>>>> + if (!LHSEleType->isIntegerType()) {
>>>>> + S.Diag(Loc, diag::err_typecheck_expect_int)
>>>>> + << LHS.get()->getType() << LHS.get()->getSourceRange();
>>>>> + return QualType();
>>>>> + }
>>>>> +
>>>>> + if (!RHSEleType->isIntegerType()) {
>>>>> + S.Diag(Loc, diag::err_typecheck_expect_int)
>>>>> + << RHS.get()->getType() << RHS.get()->getSourceRange();
>>>>> + return QualType();
>>>>> + }
>>>>> +
>>>>> + if (RHSVecTy) {
>>>>> + // OpenCL v1.1 s6.3.j says that for vector types, the operators
>>>>> + // are applied component-wise. So if RHS is a vector, then ensure
>>>>> + // that the number of elements is the same as LHS...
>>>>> + if (RHSVecTy->getNumElements() != LHSVecTy->getNumElements()) {
>>>>> + S.Diag(Loc, diag::err_typecheck_vector_lengths_not_equal)
>>>>> + << LHS.get()->getType() << RHS.get()->getType()
>>>>> + << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
>>>>> + return QualType();
>>>>> + }
>>>>> + } else {
>>>>> + // ...else expand RHS to match the number of elements in LHS.
>>>>> + QualType VecTy =
>>>>> + S.Context.getExtVectorType(RHSEleType, LHSVecTy->getNumElements());
>>>>> + RHS = S.ImpCastExprToType(RHS.get(), VecTy, CK_VectorSplat);
>>>>> + }
>>>>> +
>>>>> + return LHSType;
>>>>> +}
>>>>> +
>>>>> // C99 6.5.7
>>>>> QualType Sema::CheckShiftOperands(ExprResult &LHS, ExprResult &RHS,
>>>>> SourceLocation Loc, unsigned Opc,
>>>>> @@ -7780,8 +7841,11 @@ QualType Sema::CheckShiftOperands(ExprRe
>>>>>
>>>>> // Vector shifts promote their scalar inputs to vector type.
>>>>> if (LHS.get()->getType()->isVectorType() ||
>>>>> - RHS.get()->getType()->isVectorType())
>>>>> + RHS.get()->getType()->isVectorType()) {
>>>>> + if (LangOpts.OpenCL)
>>>>> + return checkOpenCLVectorShift(*this, LHS, RHS, Loc);
>>>>> return CheckVectorOperands(LHS, RHS, Loc, IsCompAssign);
>>>>> + }
>>>>>
>>>>> // Shifts don't perform usual arithmetic conversions, they just do integer
>>>>> // promotions on each operand. C99 6.5.7p3
>>>>>
>>>>> Modified: cfe/trunk/test/CodeGenOpenCL/shifts.cl
>>>>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenOpenCL/shifts.cl?rev=228382&r1=228381&r2=228382&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/test/CodeGenOpenCL/shifts.cl (original)
>>>>> +++ cfe/trunk/test/CodeGenOpenCL/shifts.cl Thu Feb 5 23:44:55 2015
>>>>> @@ -1,57 +1,73 @@
>>>>> -// RUN: %clang_cc1 -x cl -O1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
>>>>> -// OpenCL essentially reduces all shift amounts to the last word-size bits before evaluating.
>>>>> -// Test this both for variables and constants evaluated in the front-end.
>>>>> +// RUN: %clang_cc1 -x cl -O1 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -check-prefix=OPT
>>>>> +// RUN: %clang_cc1 -x cl -O0 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s -check-prefix=NOOPT
>>>>>
>>>>> +// OpenCL essentially reduces all shift amounts to the last word-size
>>>>> +// bits before evaluating. Test this both for variables and constants
>>>>> +// evaluated in the front-end.
>>>>> +
>>>>> +// OPT: @gtest1 = constant i64 2147483648
>>>>> +__constant const unsigned long gtest1 = 1UL << 31;
>>>>> +
>>>>> +// NOOPT: @negativeShift32
>>>>> +int negativeShift32(int a,int b) {
>>>>> + // NOOPT: %array0 = alloca [256 x i8]
>>>>> + char array0[((int)1)<<40];
>>>>> + // NOOPT: %array1 = alloca [256 x i8]
>>>>> + char array1[((int)1)<<(-24)];
>>>>>
>>>>> -//CHECK: @positiveShift32
>>>>> + // NOOPT: ret i32 65536
>>>>> + return ((int)1)<<(-16);
>>>>> +}
>>>>> +
>>>>> +//OPT: @positiveShift32
>>>>> int positiveShift32(int a,int b) {
>>>>> - //CHECK: [[M32:%.+]] = and i32 %b, 31
>>>>> - //CHECK-NEXT: [[C32:%.+]] = shl i32 %a, [[M32]]
>>>>> + //OPT: [[M32:%.+]] = and i32 %b, 31
>>>>> + //OPT-NEXT: [[C32:%.+]] = shl i32 %a, [[M32]]
>>>>> int c = a<<b;
>>>>> int d = ((int)1)<<33;
>>>>> - //CHECK-NEXT: [[E32:%.+]] = add nsw i32 [[C32]], 2
>>>>> + //OPT-NEXT: [[E32:%.+]] = add nsw i32 [[C32]], 2
>>>>> int e = c + d;
>>>>> - //CHECK-NEXT: ret i32 [[E32]]
>>>>> + //OPT-NEXT: ret i32 [[E32]]
>>>>> return e;
>>>>> }
>>>>>
>>>>> -//CHECK: @positiveShift64
>>>>> +//OPT: @positiveShift64
>>>>> long positiveShift64(long a,long b) {
>>>>> - //CHECK: [[M64:%.+]] = and i64 %b, 63
>>>>> - //CHECK-NEXT: [[C64:%.+]] = ashr i64 %a, [[M64]]
>>>>> + //OPT: [[M64:%.+]] = and i64 %b, 63
>>>>> + //OPT-NEXT: [[C64:%.+]] = ashr i64 %a, [[M64]]
>>>>> long c = a>>b;
>>>>> long d = ((long)8)>>65;
>>>>> - //CHECK-NEXT: [[E64:%.+]] = add nsw i64 [[C64]], 4
>>>>> + //OPT-NEXT: [[E64:%.+]] = add nsw i64 [[C64]], 4
>>>>> long e = c + d;
>>>>> - //CHECK-NEXT: ret i64 [[E64]]
>>>>> + //OPT-NEXT: ret i64 [[E64]]
>>>>> return e;
>>>>> }
>>>>>
>>>>> typedef __attribute__((ext_vector_type(4))) int int4;
>>>>>
>>>>> -//CHECK: @vectorVectorTest
>>>>> +//OPT: @vectorVectorTest
>>>>> int4 vectorVectorTest(int4 a,int4 b) {
>>>>> - //CHECK: [[VM:%.+]] = and <4 x i32> %b, <i32 31, i32 31, i32 31, i32 31>
>>>>> - //CHECK-NEXT: [[VC:%.+]] = shl <4 x i32> %a, [[VM]]
>>>>> + //OPT: [[VM:%.+]] = and <4 x i32> %b, <i32 31, i32 31, i32 31, i32 31>
>>>>> + //OPT-NEXT: [[VC:%.+]] = shl <4 x i32> %a, [[VM]]
>>>>> int4 c = a << b;
>>>>> - //CHECK-NEXT: [[VF:%.+]] = add <4 x i32> [[VC]], <i32 2, i32 4, i32 16, i32 8>
>>>>> + //OPT-NEXT: [[VF:%.+]] = add <4 x i32> [[VC]], <i32 2, i32 4, i32 16, i32 8>
>>>>> int4 d = {1, 1, 1, 1};
>>>>> int4 e = {33, 34, -28, -29};
>>>>> int4 f = c + (d << e);
>>>>> - //CHECK-NEXT: ret <4 x i32> [[VF]]
>>>>> + //OPT-NEXT: ret <4 x i32> [[VF]]
>>>>> return f;
>>>>> }
>>>>>
>>>>> -//CHECK: @vectorScalarTest
>>>>> +//OPT: @vectorScalarTest
>>>>> int4 vectorScalarTest(int4 a,int b) {
>>>>> - //CHECK: [[SP0:%.+]] = insertelement <4 x i32> undef, i32 %b, i32 0
>>>>> - //CHECK: [[SP1:%.+]] = shufflevector <4 x i32> [[SP0]], <4 x i32> undef, <4 x i32> zeroinitializer
>>>>> - //CHECK: [[VSM:%.+]] = and <4 x i32> [[SP1]], <i32 31, i32 31, i32 31, i32 31>
>>>>> - //CHECK-NEXT: [[VSC:%.+]] = shl <4 x i32> %a, [[VSM]]
>>>>> + //OPT: [[SP0:%.+]] = insertelement <4 x i32> undef, i32 %b, i32 0
>>>>> + //OPT: [[SP1:%.+]] = shufflevector <4 x i32> [[SP0]], <4 x i32> undef, <4 x i32> zeroinitializer
>>>>> + //OPT: [[VSM:%.+]] = and <4 x i32> [[SP1]], <i32 31, i32 31, i32 31, i32 31>
>>>>> + //OPT-NEXT: [[VSC:%.+]] = shl <4 x i32> %a, [[VSM]]
>>>>> int4 c = a << b;
>>>>> - //CHECK-NEXT: [[VSF:%.+]] = add <4 x i32> [[VSC]], <i32 4, i32 4, i32 4, i32 4>
>>>>> + //OPT-NEXT: [[VSF:%.+]] = add <4 x i32> [[VSC]], <i32 4, i32 4, i32 4, i32 4>
>>>>> int4 d = {1, 1, 1, 1};
>>>>> int4 f = c + (d << 34);
>>>>> - //CHECK-NEXT: ret <4 x i32> [[VSF]]
>>>>> + //OPT-NEXT: ret <4 x i32> [[VSF]]
>>>>> return f;
>>>>> }
>>>>>
>>>>> Modified: cfe/trunk/test/SemaOpenCL/shifts.cl
>>>>> URL:http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaOpenCL/shifts.cl?rev=228382&r1=228381&r2=228382&view=diff
>>>>> ==============================================================================
>>>>> --- cfe/trunk/test/SemaOpenCL/shifts.cl (original)
>>>>> +++ cfe/trunk/test/SemaOpenCL/shifts.cl Thu Feb 5 23:44:55 2015
>>>>> @@ -1,17 +1,54 @@
>>>>> -// RUN: %clang_cc1 -x cl -O0 -emit-llvm %s -o - -triple x86_64-linux-gnu | FileCheck %s
>>>>> -// OpenCL essentially reduces all shift amounts to the last word-size bits before evaluating.
>>>>> -// Test this both for variables and constants evaluated in the front-end.
>>>>> -
>>>>> -// CHECK: @gtest1 = constant i64 2147483648
>>>>> -__constant const unsigned long gtest1 = 1UL << 31;
>>>>> -
>>>>> -// CHECK: @negativeShift32
>>>>> -int negativeShift32(int a,int b) {
>>>>> - // CHECK: %array0 = alloca [256 x i8]
>>>>> - char array0[((int)1)<<40];
>>>>> - // CHECK: %array1 = alloca [256 x i8]
>>>>> - char array1[((int)1)<<(-24)];
>>>>> +// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
>>>>>
>>>>> - // CHECK: ret i32 65536
>>>>> - return ((int)1)<<(-16);
>>>>> +// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
>>>>> +
>>>>> +typedef __attribute__((ext_vector_type(2))) char char2;
>>>>> +typedef __attribute__((ext_vector_type(3))) char char3;
>>>>> +
>>>>> +typedef __attribute__((ext_vector_type(2))) int int2;
>>>>> +
>>>>> +typedef __attribute__((ext_vector_type(2))) float float2;
>>>>> +
>>>>> +// ** Positive tests **
>>>>> +
>>>>> +char2 ptest01(char2 c, char s) {
>>>>> + return c << s;
>>>>> +}
>>>>> +
>>>>> +char2 ptest02(char2 c, char2 s) {
>>>>> + return c << s;
>>>>> +}
>>>>> +
>>>>> +char2 ptest03(char2 c, int s) {
>>>>> + return c << s;
>>>>> +}
>>>>> +
>>>>> +char2 ptest04(char2 c, int2 s) {
>>>>> + return c << s;
>>>>> +}
>>>>> +
>>>>> +int2 ptest05(int2 c, char2 s) {
>>>>> + return c << s;
>>>>> +}
>>>>> +
>>>>> +char2 ptest06(char2 c) {
>>>>> + return c << 1;
>>>>> +}
>>>>> +
>>>>> +// ** Negative tests **
>>>>> +
>>>>> +char2 ntest01(char c, char2 s) {
>>>>> + return c << s; // expected-error {{requested shift is a vector of type 'char2' (vector of 2 'char' values) but the first operand is not a vector ('char')}}
>>>>> +}
>>>>> +
>>>>> +char3 ntest02(char3 c, char2 s) {
>>>>> + return c << s; // expected-error {{vector operands do not have the same number of elements ('char3' (vector of 3 'char' values) and 'char2' (vector of 2 'char' values))}}
>>>>> +}
>>>>> +
>>>>> +float2 ntest03(float2 c, char s) {
>>>>> + return c << s; // expected-error {{used type 'float2' (vector of 2 'float' values) where integer is required}}
>>>>> +}
>>>>> +
>>>>> +int2 ntest04(int2 c, float s) {
>>>>> + return c << s; // expected-error {{used type 'float' where integer is required}}
>>>>> }
>>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
>>
>> <vector-shift-fixed.patch>
More information about the cfe-commits
mailing list