Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c88cb12
[Float2Int] Pre-commit tests (NFC)
AZero13 Mar 20, 2024
805da06
[Float2Int] Fix miscompile with floats that can be converted to large…
AZero13 Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions llvm/lib/Transforms/Scalar/Float2Int.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,11 +407,10 @@ 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);
const APFloat &F = CF->getValueAPF();
APSInt Val(ToTy->getPrimitiveSizeInBits(), !F.isNegative());
bool Exact;
CF->getValueAPF().convertToInteger(Val,
APFloat::rmNearestTiesToEven,
&Exact);
F.convertToInteger(Val, APFloat::rmNearestTiesToEven, &Exact);
NewOperands.push_back(ConstantInt::get(ToTy, Val));
} else {
llvm_unreachable("Unhandled operand type?");
Expand Down
54 changes: 54 additions & 0 deletions llvm/test/Transforms/Float2Int/basic.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,3 +349,57 @@ bogusBB: ; preds = %bogusBB
%tobool = fcmp une double %inc, 0.000000e+00
br label %bogusBB
}

define i32 @pr79158() {
; CHECK-LABEL: @pr79158(
; 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 i64
; CHECK-NEXT: [[MUL_I2:%.*]] = mul i64 [[TMP0]], 4294967295
; CHECK-NEXT: [[MUL_I1:%.*]] = trunc i64 [[MUL_I2]] to i32
; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 4, ptr nonnull [[X_I]])
; CHECK-NEXT: ret i32 [[MUL_I1]]
;
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

34 makes more sense than 35 for UINT_MAX

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

34 makes more sense than 35 for UINT_MAX

How so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

%mul.i = fmul double %conv.i, 4294967295.0
%conv1.i = fptoui double %mul.i to i32
call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %x.i)
ret i32 %conv1.i
}

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 i64
; CHECK-NEXT: [[MUL_I2:%.*]] = mul i64 [[TMP0]], 2147483648
; CHECK-NEXT: [[MUL_I1:%.*]] = trunc i64 [[MUL_I2]] to i32
; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 4, ptr nonnull [[X_I]])
; CHECK-NEXT: ret i32 [[MUL_I1]]
;
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, 2147483648.0
%conv1.i = fptoui double %mul.i to i32
call void @llvm.lifetime.end.p0(i64 4, ptr nonnull %x.i)
ret i32 %conv1.i
}