Following John McCall's comments about testing constant sized array expressions
I found out that I also needed to modify the constant expression processing in
the front end. The updated patch attached does this. Although the engineer in
me is a little concerned that there's two widely different code-paths that need
to implement the same subtle behavioural changes, I can't see a better way of
doing this in the current architecture. The test case also includes both array
expression tests and a brief note citing where the OpenCL spec talks about the
different semantic.
Assuming no objections, I'll commit in a day or two.
________________________________________
From: [email protected] [[email protected]] On
Behalf Of David Tweed [[email protected]]
Sent: Friday, December 21, 2012 5:51 PM
To: Jordan Rose
Cc: [email protected]
Subject: Re: [cfe-commits] [PATCH] fix shifts that are defined in OpenCL but
not in C99, etc
Hi,
it's a bit complicated: the _existing_ warnings warn about things _using the C
semantics_, even going so far as to say what shifted values are (eg, 1<<37
gives 0x2000000000 which is beyond the range of the data-type). On the other
hand, with OpenCL you want to warn that 1<<37 is actually going to give the
value 1<<5 (or 0x20) which might not be what is expected. So AFAICS you either
need completely separate code with new warnings for this case, or you've got to
reduce the shift value (noting that you've done it) prior to doing the
checking, and I gather changing values is something which shouldn't be being
done in the Sema layer.
Regards,
Dave
________________________________________
From: Jordan Rose [[email protected]]
Sent: Friday, December 21, 2012 5:13 PM
To: David Tweed
Cc: [email protected]
Subject: Re: [cfe-commits] [PATCH] fix shifts that are defined in OpenCL but
not in C99, etc
I don't have much context here, but it seems like the warnings are still valid
in OpenCL (because of the surprise factor); they might just be off by default.
Or, since we don't like off-by-default warnings, they would at least have
different warning text to make it clear that it's not undefined behavior, just
something the programmer might not expect.
On Dec 21, 2012, at 0:27 , David Tweed <[email protected]> wrote:
> Hi,
>
> One of the corner cases for C family languages is shifts (<< and >>) where
> the shift size is not with 0 to "word width". In C99 (and I think most other
> C variants) these are undefined while OpenCL defines the meaning to be along
> the lines of "integer promote shift variable as required, promote the shift
> amount the same way and use the bottom promoted-type-width bits as the shift
> amount" (this applies even if the shift amount is originally a negative
> number). (Full technical description is in section 6.3j of OpenCL spec.) This
> patch implements two parts to this; actually generating the IR in clang is
> straightforward. The difficult bit is the front-end semantic diagnostics when
> values are known statically. If in OpenCL I something expands to "1<<37" or
> "1<<(-4)" it's definitely a well defined program fargment, but should any
> judgement be made as to if it is likely to do what the programmer intended?
> At the moment I've taken the view that, particularly since Sema shouldn't ac!
tually change values by reducing the shift, it's not possible to generate
meaningful warnings for this construct in OpenCL, so it returns before most of
the bad shift checking in SemaExpr.cpp's DiagnoseBadShiftValues?
>
> Could this be reviewed and then I'll commit it please?
>
> Cheers,
> Dave
>
> -- IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended
> recipient, please notify the sender immediately and do not disclose the
> contents to any other person, use it for any purpose, or store or copy the
> information in any medium. Thank
> you.<shiftSize2.diff>_______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
-- IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
diff --git lib/AST/ExprConstant.cpp lib/AST/ExprConstant.cpp
index e9f1f48..f4f24b1 100644
--- lib/AST/ExprConstant.cpp
+++ lib/AST/ExprConstant.cpp
@@ -4708,9 +4708,12 @@ bool DataRecursiveIntBinOpEvaluator::
return Success(E->getOpcode() == BO_Rem ? LHS % RHS : LHS / RHS, E,
Result);
case BO_Shl: {
- // During constant-folding, a negative shift is an opposite shift. Such
- // a shift is not a constant expression.
- if (RHS.isSigned() && RHS.isNegative()) {
+ if (Info.getLangOpts().OpenCL)
+ //OpenCL 6.3j: shift values are effectively % word size of LHS
+ RHS &= APSInt(llvm::APInt(LHS.getBitWidth(), static_cast<uint64_t>(LHS.getBitWidth() - 1), RHS.isSigned()), RHS.isUnsigned());
+ else if (RHS.isSigned() && RHS.isNegative()) {
+ // During constant-folding, a negative shift is an opposite shift. Such
+ // a shift is not a constant expression.
CCEDiag(E, diag::note_constexpr_negative_shift) << RHS;
RHS = -RHS;
goto shift_right;
@@ -4735,9 +4738,12 @@ bool DataRecursiveIntBinOpEvaluator::
return Success(LHS << SA, E, Result);
}
case BO_Shr: {
- // During constant-folding, a negative shift is an opposite shift. Such a
- // shift is not a constant expression.
- if (RHS.isSigned() && RHS.isNegative()) {
+ if (Info.getLangOpts().OpenCL)
+ //OpenCL 6.3j: shift values are effectively % word size of LHS
+ RHS &= APSInt(llvm::APInt(LHS.getBitWidth(), static_cast<uint64_t>(LHS.getBitWidth() - 1), RHS.isSigned()), RHS.isUnsigned());
+ else if (RHS.isSigned() && RHS.isNegative()) {
+ // During constant-folding, a negative shift is an opposite shift. Such a
+ // shift is not a constant expression.
CCEDiag(E, diag::note_constexpr_negative_shift) << RHS;
RHS = -RHS;
goto shift_left;
diff --git lib/CodeGen/CGExprScalar.cpp lib/CodeGen/CGExprScalar.cpp
index 50428e6..98c5ce3 100644
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -429,6 +429,8 @@ public:
// Check for undefined division and modulus behaviors.
void EmitUndefinedBehaviorIntegerDivAndRemCheck(const BinOpInfo &Ops,
llvm::Value *Zero,bool isDiv);
+ // Common helper for getting how wide LHS of shift is.
+ static Value *GetWidthMinusOneValue(Value* LHS,Value* RHS);
Value *EmitDiv(const BinOpInfo &Ops);
Value *EmitRem(const BinOpInfo &Ops);
Value *EmitAdd(const BinOpInfo &Ops);
@@ -2365,6 +2367,11 @@ Value *ScalarExprEmitter::EmitSub(const BinOpInfo &op) {
return Builder.CreateExactSDiv(diffInChars, divisor, "sub.ptr.div");
}
+Value *ScalarExprEmitter::GetWidthMinusOneValue(Value* LHS,Value* RHS) {
+ unsigned Width = cast<llvm::IntegerType>(LHS->getType())->getBitWidth();
+ return llvm::ConstantInt::get(RHS->getType(), Width - 1);
+}
+
Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
// LLVM requires the LHS and RHS to be the same type: promote or truncate the
// RHS to the same size as the LHS.
@@ -2374,9 +2381,7 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
if (CGF.getLangOpts().SanitizeShift &&
isa<llvm::IntegerType>(Ops.LHS->getType())) {
- unsigned Width = cast<llvm::IntegerType>(Ops.LHS->getType())->getBitWidth();
- llvm::Value *WidthMinusOne =
- llvm::ConstantInt::get(RHS->getType(), Width - 1);
+ llvm::Value *WidthMinusOne = GetWidthMinusOneValue(Ops.LHS, RHS);
// FIXME: Emit the branching explicitly rather than emitting the check
// twice.
EmitBinOpCheck(Builder.CreateICmpULE(RHS, WidthMinusOne), Ops);
@@ -2401,6 +2406,9 @@ Value *ScalarExprEmitter::EmitShl(const BinOpInfo &Ops) {
EmitBinOpCheck(Builder.CreateICmpEQ(BitsShiftedOff, Zero), Ops);
}
}
+ // OpenCL 6.3j: shift values are effectively % word size of LHS.
+ if (CGF.getLangOpts().OpenCL)
+ RHS = Builder.CreateAnd(RHS, GetWidthMinusOneValue(Ops.LHS, RHS), "shl.mask");
return Builder.CreateShl(Ops.LHS, RHS, "shl");
}
@@ -2414,10 +2422,11 @@ Value *ScalarExprEmitter::EmitShr(const BinOpInfo &Ops) {
if (CGF.getLangOpts().SanitizeShift &&
isa<llvm::IntegerType>(Ops.LHS->getType())) {
- unsigned Width = cast<llvm::IntegerType>(Ops.LHS->getType())->getBitWidth();
- llvm::Value *WidthVal = llvm::ConstantInt::get(RHS->getType(), Width);
- EmitBinOpCheck(Builder.CreateICmpULT(RHS, WidthVal), Ops);
+ EmitBinOpCheck(Builder.CreateICmpULE(RHS, GetWidthMinusOneValue(Ops.LHS, RHS)), Ops);
}
+ // OpenCL 6.3j: shift values are effectively % word size of LHS.
+ if (CGF.getLangOpts().OpenCL)
+ RHS = Builder.CreateAnd(RHS, GetWidthMinusOneValue(Ops.LHS, RHS), "shr.mask");
if (Ops.Ty->hasUnsignedIntegerRepresentation())
return Builder.CreateLShr(Ops.LHS, RHS, "shr");
diff --git lib/Sema/SemaExpr.cpp lib/Sema/SemaExpr.cpp
index e573a15..9d7253a 100644
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -6584,6 +6584,11 @@ static void DiagnoseBadShiftValues(Sema& S, ExprResult &LHS, ExprResult &RHS,
!RHS.get()->isIntegerConstantExpr(Right, S.Context))
return;
+ //OpenCL 6.3j: shift values are effectively % word size of LHS (more defined),
+ //so skip remaining warnings as we don't want to modify values within Sema.
+ if (S.getLangOpts().OpenCL)
+ return;
+
if (Right.isNegative()) {
S.DiagRuntimeBehavior(Loc, RHS.get(),
S.PDiag(diag::warn_shift_negative)
diff --git test/CodeGen/catch-undef-behavior.c test/CodeGen/catch-undef-behavior.c
index d0b6d19..9170666 100644
--- test/CodeGen/catch-undef-behavior.c
+++ test/CodeGen/catch-undef-behavior.c
@@ -99,7 +99,7 @@ int lsh_overflow(int a, int b) {
// CHECK: @rsh_inbounds
int rsh_inbounds(int a, int b) {
- // CHECK: %[[INBOUNDS:.*]] = icmp ult i32 %[[RHS:.*]], 32
+ // CHECK: %[[INBOUNDS:.*]] = icmp ule i32 %[[RHS:.*]], 31
// CHECK: br i1 %[[INBOUNDS]]
// CHECK: %[[ARG1:.*]] = zext
diff --git test/CodeGenOpenCL/shifts.cl test/CodeGenOpenCL/shifts.cl
new file mode 100644
index 0000000..c08f4e4
--- /dev/null
+++ test/CodeGenOpenCL/shifts.cl
@@ -0,0 +1,39 @@
+// 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.
+
+//CHECK: @array0 = common global [8 x i8]
+char array0[1<<35];
+//CHECK: @array1 = common global [16 x i8]
+char array1[1<<(-29)];
+
+//CHECK: @positiveShift32
+int positiveShift32(int a,int b) {
+ //CHECK: %shl.mask = and i32 %b, 31
+ //CHECK-NEXT: %shl = shl i32 %a, %shl.mask
+ int c = a<<b;
+ int d = ((int)1)<<33;
+ //CHECK-NEXT: %add = add nsw i32 %shl, 2
+ int e = c + d;
+ //CHECK-NEXT: ret i32 %add
+ return e;
+}
+
+//CHECK: @negativeShift32
+int negativeShift32(int a,int b) {
+ //CHECK: ret i32 65536
+ return ((int)1)<<(-16);
+}
+
+//CHECK: @positiveShift64
+long positiveShift64(long a,long b) {
+ //CHECK: %shr.mask = and i64 %b, 63
+ //CHECK-NEXT: %shr = ashr i64 %a, %shr.mask
+ long c = a>>b;
+ long d = ((long)8)>>65;
+ //CHECK-NEXT: %add = add nsw i64 %shr, 4
+ long e = c + d;
+ //CHECK-NEXT: ret i64 %add
+ return e;
+}
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
