In ExprConstant.cpp (2x):
+ 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());
Add a space after '//', and add a full stop to the comment. Wrap the
last line to 80 columns. The first argument to the APInt constructor
should be RHS.getBitWidth(), not LHS.getBitWidth(). Drop the third
argument to the APInt constructor (it never actually matters, but you
would want to zero-extend the mask, not sign-extend it).
CGExprScalar.cpp:
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);
}
Remove braces here.
+ // 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");
Presumably, -fsanitize=shift should be disabled for OpenCL. It's
included in -fsanitize=undefined, which should only catch undefined
behavior.
In SemaExpr.cpp:
+ //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;
Perform this check before we evaluate the RHS.
We also like to test behavioral changes as early in the compilation
flow as possible, so a Sema test for the constant expression changes
would be great!
On Thu, Jan 3, 2013 at 7:10 AM, David Tweed <[email protected]> wrote:
> 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 a!
c!
> 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.
>
> _______________________________________________
> cfe-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
