[llvm] r219542 - Return undef on FP <-> Int conversions that overflow (PR21330).
Richard Smith
richard at metafoo.co.uk
Fri Oct 17 18:49:26 PDT 2014
On Fri, Oct 17, 2014 at 5:17 PM, Quentin Colombet <qcolombet at apple.com>
wrote:
> Hi Sanjay,
>
> I do not think this change is correct.
> I have attached a test case with (before.ll) and without (after.ll) that
> change and the related C file (test.c).
>
signed short a = 8*8*8*548.54 - 3.14; // <-- the value of a is undef.
This line has undefined behavior; both C and C++ are very clear about this.
The transformation is correct at the language semantics level. And it
appears correct at the IR level too, whether you interpret "the results are
undefined" as meaning undefined behavior or (strictly weaker) an undef
value. It would be correct to treat this as poison, not just as undef, from
a language semantics perspective.
To reproduce, compile with O1 or more and run the produced executable. The
> correct behavior is do not assert :).
>
> Please revert or fix it.
>
> On Oct 10, 2014, at 4:00 PM, Sanjay Patel <spatel at rotateright.com> wrote:
>
> Author: spatel
> Date: Fri Oct 10 18:00:21 2014
> New Revision: 219542
>
> URL: http://llvm.org/viewvc/llvm-project?rev=219542&view=rev
> Log:
> Return undef on FP <-> Int conversions that overflow (PR21330).
>
> The LLVM Lang Ref states for signed/unsigned int to float conversions:
> "If the value cannot fit in the floating point value, the results are
> undefined."
>
> And for FP to signed/unsigned int:
> "If the value cannot fit in ty2, the results are undefined."
>
> This matches the C definitions.
>
>
> Yes, this matches the C definitions, but the rules of propagating the
> undef value are not strictly the same in C and LLVM IR AFAICT.
> Here is an example:
> In (pseudo) C,
> a = undef // a is undef;
> if (something)
> b = val;
> else
> b = a; // b is undef, but it should be the same value as a.
> assert (something || b == a);
>
> The related (pseudo) IR would be:
> a = undef
> b = select i1 something, val, a
> cmpVal = cmp eq b, a
> assertVal = or something, cmpVal
> call assert(assertVal)
>
> Now, the actual IR will be, because of constant propagation:
> b = select i1 something, val, undef // <— we lose the information that
> the next two undef are the same value.
> cmpVal = cmp eq b, undef
> assertVal = or something, cmpVal
> call assert(assertVal)
>
> This is still fine as long as undef are expanded into the same constant.
> However, the LangRef states that the following transformation is valid:
>
> %C = select %X, %Y, undefSafe:
> %C = %YUnsafe:
> %C = undef
>
> Therefore, we end up with the following IR:
> b = val // <— now, the value of b may not be a when ‘something' is false,
> which is incorrect.
> cmpVal = cmp eq b, undef
> assertVal = or something, cmpVal
> call assert(assertVal)
>
> Thanks,
> -Quentin
>
>
>
>
>
> The existing behavior pins to infinity or a max int value, but that may
> just
> lead to more confusion as seen in:
> http://llvm.org/bugs/show_bug.cgi?id=21130
>
> Returning undef will hopefully lead to a less silent failure.
>
> Differential Revision: http://reviews.llvm.org/D5603
>
>
> Modified:
> llvm/trunk/lib/IR/ConstantFold.cpp
> llvm/trunk/test/Transforms/InstCombine/cast.ll
>
> Modified: llvm/trunk/lib/IR/ConstantFold.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/ConstantFold.cpp?rev=219542&r1=219541&r2=219542&view=diff
>
> ==============================================================================
> --- llvm/trunk/lib/IR/ConstantFold.cpp (original)
> +++ llvm/trunk/lib/IR/ConstantFold.cpp Fri Oct 10 18:00:21 2014
> @@ -593,8 +593,13 @@ Constant *llvm::ConstantFoldCastInstruct
> bool ignored;
> uint64_t x[2];
> uint32_t DestBitWidth = cast<IntegerType>(DestTy)->getBitWidth();
> - (void) V.convertToInteger(x, DestBitWidth, opc==Instruction::FPToSI,
> - APFloat::rmTowardZero, &ignored);
> + if (APFloat::opInvalidOp ==
> + V.convertToInteger(x, DestBitWidth, opc==Instruction::FPToSI,
> + APFloat::rmTowardZero, &ignored)) {
> + // Undefined behavior invoked - the destination type can't
> represent
> + // the input constant.
> + return UndefValue::get(DestTy);
> + }
> APInt Val(DestBitWidth, x);
> return ConstantInt::get(FPC->getContext(), Val);
> }
> @@ -653,9 +658,13 @@ Constant *llvm::ConstantFoldCastInstruct
> APInt api = CI->getValue();
> APFloat apf(DestTy->getFltSemantics(),
> APInt::getNullValue(DestTy->getPrimitiveSizeInBits()));
> - (void)apf.convertFromAPInt(api,
> - opc==Instruction::SIToFP,
> - APFloat::rmNearestTiesToEven);
> + if (APFloat::opOverflow &
> + apf.convertFromAPInt(api, opc==Instruction::SIToFP,
> + APFloat::rmNearestTiesToEven)) {
> + // Undefined behavior invoked - the destination type can't
> represent
> + // the input constant.
> + return UndefValue::get(DestTy);
> + }
> return ConstantFP::get(V->getContext(), apf);
> }
> return nullptr;
>
> Modified: llvm/trunk/test/Transforms/InstCombine/cast.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/cast.ll?rev=219542&r1=219541&r2=219542&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/Transforms/InstCombine/cast.ll (original)
> +++ llvm/trunk/test/Transforms/InstCombine/cast.ll Fri Oct 10 18:00:21 2014
> @@ -1050,3 +1050,37 @@ define i8 @test85(i32 %a) {
> ; CHECK: [[SHR:%.*]] = lshr exact i32 [[ADD]], 23
> ; CHECK: [[CST:%.*]] = trunc i32 [[SHR]] to i8
> }
> +
> +; Overflow on a float to int or int to float conversion is undefined
> (PR21130).
> +
> +define i8 @overflow_fptosi() {
> + %i = fptosi double 1.56e+02 to i8
> + ret i8 %i
> +; CHECK-LABEL: @overflow_fptosi(
> +; CHECK-NEXT: ret i8 undef
> +}
> +
> +define i8 @overflow_fptoui() {
> + %i = fptoui double 2.56e+02 to i8
> + ret i8 %i
> +; CHECK-LABEL: @overflow_fptoui(
> +; CHECK-NEXT: ret i8 undef
> +}
> +
> +; The maximum float is approximately 2 ** 128 which is 3.4E38.
> +; The constant below is 4E38. Use a 130 bit integer to hold that
> +; number; 129-bits for the value + 1 bit for the sign.
> +define float @overflow_uitofp() {
> + %i = uitofp i130 400000000000000000000000000000000000000 to float
> + ret float %i
> +; CHECK-LABEL: @overflow_uitofp(
> +; CHECK-NEXT: ret float undef
> +}
> +
> +define float @overflow_sitofp() {
> + %i = sitofp i130 400000000000000000000000000000000000000 to float
> + ret float %i
> +; CHECK-LABEL: @overflow_sitofp(
> +; CHECK-NEXT: ret float undef
> +}
> +
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141017/37ed1c02/attachment.html>
More information about the llvm-commits
mailing list