[Float2Int] Fix miscompile with floats that can be converted to large values

[AZero13]

There is an issue with Float2Int whenever it has to deal with floats that evaluate to numbers that are over INT_MAX, but under UINT_MAX.

Simply, because the conversion method always assumes a signed integer, this works until the 32 bit number is actually a positive 32 bit number that would be interpreted as a negative number when evaluated as an integer, being put in an i32.

This was messing up the Float2Int conversions and causing miscompiles.

To fix this, tell the APSInt constructor if the value is positive or negative.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: AtariDreams (AtariDreams)

Changes

There is an issue with Float2Int whenever it has to deal with floats that evaluate to numbers that are over INT_MAX, but under UINT_MAX.

Simply, because the conversion method always assumes a signed integer, this works until the 32 bit number is actually a positive 32 bit number that would be interpreted as a negative number when evaluated as an integer, being put in an i32.

This was messing up the Float2Int conversions and causing miscompiles.

To fix this, tell the APSInt constructor if the value is positive or negative.

---

Full diff: https://github.com/llvm/llvm-project/pull/85996.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/Float2Int.cpp (+2-1)
• (modified) llvm/test/Transforms/Float2Int/basic.ll (+26)

diff --git a/llvm/lib/Transforms/Scalar/Float2Int.cpp b/llvm/lib/Transforms/Scalar/Float2Int.cpp
index ccca8bcc1a56ac..dc72a65c618053 100644
--- a/llvm/lib/Transforms/Scalar/Float2Int.cpp
+++ b/llvm/lib/Transforms/Scalar/Float2Int.cpp
@@ -409,7 +409,8 @@ Value *Float2IntPass::convert(Instruction *I, Type *ToTy) {
     } else if (Instruction *VI = dyn_cast<Instruction>(V)) {
       NewOperands.push_back(convert(VI, ToTy));
     } else if (ConstantFP *CF = dyn_cast<ConstantFP>(V)) {
-      APSInt Val(ToTy->getPrimitiveSizeInBits(), /*isUnsigned=*/false);
+      APSInt Val(ToTy->getPrimitiveSizeInBits(),
+                 !CF->getValueAPF().isNegative());
       bool Exact;
       CF->getValueAPF().convertToInteger(Val,
                                          APFloat::rmNearestTiesToEven,
diff --git a/llvm/test/Transforms/Float2Int/basic.ll b/llvm/test/Transforms/Float2Int/basic.ll
index 2854a83179b7eb..a35c9cd2f3c3e0 100644
--- a/llvm/test/Transforms/Float2Int/basic.ll
+++ b/llvm/test/Transforms/Float2Int/basic.ll
@@ -349,3 +349,29 @@ bogusBB:                                          ; preds = %bogusBB
   %tobool = fcmp une double %inc, 0.000000e+00
   br label %bogusBB
 }
+
+define i32 @pr71958() {
+; CHECK-LABEL: @pr71958(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[X_I:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    call void @llvm.lifetime.start.p0(i64 4, ptr nonnull [[X_I]])
+; CHECK-NEXT:    store volatile i32 1, ptr [[X_I]], align 4
+; CHECK-NEXT:    [[X_I_0_X_I_0_X_0_X_0_X_0__I:%.*]] = load volatile i32, ptr [[X_I]], align 4
+; CHECK-NEXT:    [[CMP_I:%.*]] = icmp sgt i32 [[X_I_0_X_I_0_X_0_X_0_X_0__I]], 0
+; CHECK-NEXT:    [[TMP0:%.*]] = zext i1 [[CMP_I]] to i32
+; CHECK-NEXT:    [[MUL_I1:%.*]] = mul i32 [[TMP0]], -1
+; CHECK-NEXT:    call void @llvm.lifetime.end.p0(i64 4, ptr nonnull [[X_I]])
+; CHECK-NEXT:    ret i32 0
+;
+entry:
+  %x.i = alloca i32, align 4
+  call void @llvm.lifetime.start.p0(i64 4, ptr nonnull %x.i)
+  store volatile i32 1, ptr %x.i, align 4
+  %x.i.0.x.i.0.x.0.x.0.x.0..i = load volatile i32, ptr %x.i, align 4
+  %cmp.i = icmp sgt i32 %x.i.0.x.i.0.x.0.x.0.x.0..i, 0
+  %conv.i = uitofp i1 %cmp.i to double
+  %mul.i = fmul double %conv.i, 0x41EFFFFFFFE00000
+  %conv1.i = fptoui double %mul.i to i32
+  call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %x.i)
+  ret i32 0
+}

[topperc]

The range in the debug print for this instruction seems problematic

F2I:   %conv.i = uitofp i1 %cmp.i to double:[0,1)

That range only contains one value, 0.

The pass computed a minimum bitwidth of 3

F2I: MinBitwidth=3, R: [0,1)

[AZero13]

The range in the debug print for this instruction seems problematic

F2I:   %conv.i = uitofp i1 %cmp.i to double:[0,1)

That range only contains one value, 0.

The pass computed a minimum bitwidth of 3

F2I: MinBitwidth=3, R: [0,1)

Don't know if that is something for another PR because I only touched the conversion aspect, upon which the type converted to was set to 32 bits anyway

[AZero13]

Whether minbitwith is 3 or 1, it only matters in choosing a 32 or 64 bit type. At least, right now that is what happens.

[AZero13]

The range in the debug print for this instruction seems problematic

F2I:   %conv.i = uitofp i1 %cmp.i to double:[0,1)

That range only contains one value, 0.

The pass computed a minimum bitwidth of 3

F2I: MinBitwidth=3, R: [0,1)

That means either R.getLower().getSignificantBits() or R.getHigher().getSignificantBits() returned 2.

Because that + 1 makes 3

[AZero13]

If the value is not negative, this
function returns the same value as getActiveBits()+1.

Okay so assuming 1 has 1 active bit, that makes 2, and then + 1 is 3 for MinBitWidth

[AZero13]

Perhaps I should remove the + 1 from the MinBW assigned to it from the max of the range ends

[topperc]

Perhaps I should remove the + 1 from the MinBW assigned to it from the max of the range ends

Why?

[AZero13]

34 makes more sense than 35 for UINT_MAX

[topperc]

34 makes more sense than 35 for UINT_MAX

How so?

[AZero13]

define i32 @pr79158_2() {
; CHECK-LABEL: @pr79158_2(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[X_I:%.]] = alloca i32, align 4
; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 4, ptr nonnull [[X_I]])
; CHECK-NEXT: store volatile i32 1, ptr [[X_I]], align 4
; CHECK-NEXT: [[X_I_0_X_I_0_X_0_X_0_X_0__I:%.]] = load volatile i32, ptr [[X_I]], align 4
; CHECK-NEXT: [[CMP_I:%.]] = icmp sgt i32 [[X_I_0_X_I_0_X_0_X_0_X_0__I]], 0
; CHECK-NEXT: [[TMP0:%.]] = zext i1 [[CMP_I]] to i32
; CHECK-NEXT: [[MUL_I1:%.*]] = mul i32 [[TMP0]], -2147483648
; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 4, ptr nonnull [[X_I]])
; CHECK-NEXT: ret i32 0

I feel like i32 should be chosen in particular I mean.

[topperc]

I do not believe this patch is correct.