[IndVarSimplify] Ensure fp values can be represented as consecutive integers #166649
Conversation
antoniofrighetto
requested review from
dtcxzyw,
efriedma-quic,
fhahn and
nikic
|
@llvm/pr-subscribers-llvm-transforms Author: Antonio Frighetto (antoniofrighetto) ChangesWhen transforming floating-point induction variables into integer ones, make sure we stay within the bounds of fp values that can be represented as integers without gaps, i.e., 2^24 and 2^53 for IEEE-754 single and double precision respectively (both on negative and positive side). Fixes: #166496. Full diff: https://github.com/llvm/llvm-project/pull/166649.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
index 4ba4ba3850e58..0df8670942212 100644
--- a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
+++ b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
@@ -228,6 +228,21 @@ bool IndVarSimplify::handleFloatingPointIV(Loop *L, PHINode *PN) {
!ConvertToSInt(IncValueVal->getValueAPF(), IncValue))
return false;
+ // Ensure we stay within the bounds of fp values that can be represented
+ // as integers without gaps, i.e., 2^24 and 2^53 for IEEE-754 single and
+ // double precision respectively (both on negative and positive side).
+ const auto &SVFltSema = InitValueVal->getValueAPF().getSemantics();
+ if (!APFloat::isIEEELikeFP(SVFltSema))
+ return false;
+
+ uint64_t StartValPrecision = APFloat::semanticsPrecision(SVFltSema);
+ if (StartValPrecision >= 64)
+ return false;
+
+ uint64_t StartValIntegerLimit = 1LL << StartValPrecision;
+ if (uint64_t(std::abs(InitValue)) > StartValIntegerLimit)
+ return false;
+
// Check Incr uses. One user is PN and the other user is an exit condition
// used by the conditional terminator.
Value::user_iterator IncrUse = Incr->user_begin();
@@ -265,6 +280,18 @@ bool IndVarSimplify::handleFloatingPointIV(Loop *L, PHINode *PN) {
!ConvertToSInt(ExitValueVal->getValueAPF(), ExitValue))
return false;
+ const auto &EVFltSema = ExitValueVal->getValueAPF().getSemantics();
+ if (!APFloat::isIEEELikeFP(EVFltSema))
+ return false;
+
+ uint64_t ExitValPrecision = APFloat::semanticsPrecision(EVFltSema);
+ if (ExitValPrecision >= 64)
+ return false;
+
+ uint64_t ExitValIntegerLimit = 1LL << ExitValPrecision;
+ if (uint64_t(std::abs(ExitValue)) > ExitValIntegerLimit)
+ return false;
+
// Find new predicate for integer comparison.
CmpInst::Predicate NewPred = CmpInst::BAD_ICMP_PREDICATE;
switch (Compare->getPredicate()) {
diff --git a/llvm/test/Transforms/IndVarSimplify/floating-point-iv.ll b/llvm/test/Transforms/IndVarSimplify/floating-point-iv.ll
index b1ef50382c070..e3e6f5e1d2a6f 100644
--- a/llvm/test/Transforms/IndVarSimplify/floating-point-iv.ll
+++ b/llvm/test/Transforms/IndVarSimplify/floating-point-iv.ll
@@ -417,3 +417,86 @@ loop:
exit:
ret void
}
+
+define void @test_fp_to_int_irrealizable_initval() {
+; CHECK-LABEL: @test_fp_to_int_irrealizable_initval(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: [[IV:%.*]] = phi float [ 1.000000e+08, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP]] ]
+; CHECK-NEXT: call void @opaque()
+; CHECK-NEXT: [[IV_NEXT]] = fadd float [[IV]], -1.700000e+01
+; CHECK-NEXT: [[CMP:%.*]] = fcmp ult float [[IV_NEXT]], 2.500000e+01
+; CHECK-NEXT: br i1 [[CMP]], label [[EXIT:%.*]], label [[LOOP]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %loop
+
+loop:
+ %iv = phi float [ 1.000000e+08, %entry ], [ %iv.next, %loop ]
+ call void @opaque()
+ %iv.next = fadd float %iv, -1.700000e+01
+ %cmp = fcmp ult float %iv.next, 2.500000e+01
+ br i1 %cmp, label %exit, label %loop
+
+exit:
+ ret void
+}
+
+define void @test_fp_to_int_irrealizable_exitval() {
+; CHECK-LABEL: @test_fp_to_int_irrealizable_exitval(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: [[IV:%.*]] = phi float [ 2.500000e+01, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP]] ]
+; CHECK-NEXT: call void @opaque()
+; CHECK-NEXT: [[IV_NEXT]] = fadd float [[IV]], 1.700000e+01
+; CHECK-NEXT: [[CMP:%.*]] = fcmp ugt float [[IV_NEXT]], 1.000000e+08
+; CHECK-NEXT: br i1 [[CMP]], label [[EXIT:%.*]], label [[LOOP]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %loop
+
+loop:
+ %iv = phi float [ 2.500000e+01, %entry ], [ %iv.next, %loop ]
+ call void @opaque()
+ %iv.next = fadd float %iv, 1.700000e+01
+ %cmp = fcmp ugt float %iv.next, 1.000000e+08
+ br i1 %cmp, label %exit, label %loop
+
+exit:
+ ret void
+}
+
+define void @test_fp_to_int_irrealizable_negative_exitval() {
+; CHECK-LABEL: @test_fp_to_int_irrealizable_negative_exitval(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: [[IV:%.*]] = phi float [ -2.500000e+01, [[ENTRY:%.*]] ], [ [[IV_NEXT:%.*]], [[LOOP]] ]
+; CHECK-NEXT: call void @opaque()
+; CHECK-NEXT: [[IV_NEXT]] = fadd float [[IV]], -1.700000e+01
+; CHECK-NEXT: [[CMP:%.*]] = fcmp ult float [[IV_NEXT]], -1.000000e+08
+; CHECK-NEXT: br i1 [[CMP]], label [[EXIT:%.*]], label [[LOOP]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %loop
+
+loop:
+ %iv = phi float [ -2.500000e+01, %entry ], [ %iv.next, %loop ]
+ call void @opaque()
+ %iv.next = fadd float %iv, -1.700000e+01
+ %cmp = fcmp ult float %iv.next, -1.000000e+08
+ br i1 %cmp, label %exit, label %loop
+
+exit:
+ ret void
+}
+
+declare void @opaque()
|
| return false; | ||
|
|
||
| uint64_t ExitValIntegerLimit = 1LL << ExitValPrecision; | ||
| if (uint64_t(std::abs(ExitValue)) > ExitValIntegerLimit) |
There was a problem hiding this comment.
Can ExitValue be INT64_MIN ? If so then there would be undefined behavior here. I don't think it can be but it definitely deserves a comment.
There was a problem hiding this comment.
No, INT64_MIN is properly filtered out.
IEEE-754 stores the fractional component as its absolute value with a separate sign bit. It is made signed by typical twos-complement negation. In addition to the fraction is an implicit 1 (for regular) or 0 (for subnormal) left of the decimal point. The number of bits in the fraction plus the implicit digit is the precision field, which is 53 for f64 or 24 for f32.
To get ExitValue == INT64_MIN from ConvertToSInt, it must be normal (implicit 1) with 63 zeros in the fraction. Such a float would have a semantics with precision of 64, which is filtered out by ExitValPrecision >= 64. ExitValPrecision > 64 implies !ConvertToSInt, so that's redundant, but okay to keep.
And the reason that uint64_t(std::abs(ExitValue)) > (1LL << ExitValPrecision) is appropriate, as opposed to an asymmetric range where there's one more negative value allowed, is because the sign bit means there's exactly as many negative floats as positive.
However, I believe the test should be uint64_t(std::abs(ExitValue)) >= (1LL << ExitValPrecision), as equality would allow for two values (positive and negative) with precision ExitValPrecision+1.
The only float semantics with precision==64 is semX87DoubleExtended, but it's !isIEEELikeFP, so it doesn't matter that any floats which take exactly 64 bits to represent are forbidden.
Looks good, besides the >=.
There was a problem hiding this comment.
It turns out that ConvertToSInt() would indeed convert the IEEE-754 double precision representation of INT64_MIN (i.e., 0xC3E0000000000000) into an INT64_MIN integer, leading std::abs to be UB. Switched to use llvm::AbsoluteValue, which should be well-defined for all the inputs, also added a test.
However, I believe the test should be uint64_t(std::abs(ExitValue)) >= (1LL << ExitValPrecision), as equality would allow for two values (positive and negative) with precision ExitValPrecision+1.
The end sides (-2^24 and 2^24 for single precision) are the largest values a floating point can be represented as integer without gaps, so allowing them to be converted to integer should be fine. That said, the handling is currently later blocked by !isInt<32>(InitValue) check.
There was a problem hiding this comment.
Looks good to me, aside from >/>=.
When John reported the miscompilation, I drafted an approach which computes a closed form solution for the trip count with finite floats, taking into account precision loss. But it's more complex than this and should not block this PR. I may still contribute it later.
| return false; | ||
|
|
||
| uint64_t ExitValIntegerLimit = 1LL << ExitValPrecision; | ||
| if (uint64_t(std::abs(ExitValue)) > ExitValIntegerLimit) |
There was a problem hiding this comment.
No, INT64_MIN is properly filtered out.
IEEE-754 stores the fractional component as its absolute value with a separate sign bit. It is made signed by typical twos-complement negation. In addition to the fraction is an implicit 1 (for regular) or 0 (for subnormal) left of the decimal point. The number of bits in the fraction plus the implicit digit is the precision field, which is 53 for f64 or 24 for f32.
To get ExitValue == INT64_MIN from ConvertToSInt, it must be normal (implicit 1) with 63 zeros in the fraction. Such a float would have a semantics with precision of 64, which is filtered out by ExitValPrecision >= 64. ExitValPrecision > 64 implies !ConvertToSInt, so that's redundant, but okay to keep.
And the reason that uint64_t(std::abs(ExitValue)) > (1LL << ExitValPrecision) is appropriate, as opposed to an asymmetric range where there's one more negative value allowed, is because the sign bit means there's exactly as many negative floats as positive.
However, I believe the test should be uint64_t(std::abs(ExitValue)) >= (1LL << ExitValPrecision), as equality would allow for two values (positive and negative) with precision ExitValPrecision+1.
The only float semantics with precision==64 is semX87DoubleExtended, but it's !isIEEELikeFP, so it doesn't matter that any floats which take exactly 64 bits to represent are forbidden.
Looks good, besides the >=.
Wait semX87DoubleExtended is !isIEEELikeFP, that seems wrong to me. But what do I know about the LLVM internals. So I just looked and yes LLVM says semX87DoubleExtended is not isIEEELikeFP. I wonder how much fall out would happen if that was corrected :). |
dtcxzyw
left a comment
dtcxzyw
left a comment
There was a problem hiding this comment.
This constraint is still not enough.
Consider the following case:
#include <cstdio>
#include <cstdlib>
int main() {
float end = 1U << 24;
float beg = 0.0;
unsigned count = 0;
for (float i = beg; i <= end; i++) {
if (count > (1U << 26)) exit(1);
++count;
}
printf("%u\n", count);
return 0;
}
Note that (float)(1U << 24) + 1.0f == (float)(1U << 24). So the program should call exit(1) instead of exiting the loop.
LLVM IR:
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-linux-gnu"
@.str = private unnamed_addr constant [4 x i8] c"%u\0A\00", align 1
define dso_local noundef i32 @main() {
br label %1
1: ; preds = %10, %0
%2 = phi i32 [ 0, %0 ], [ %11, %10 ]
%3 = phi float [ 0.000000e+00, %0 ], [ %12, %10 ]
%4 = fcmp ugt float %3, 0x4170000000000000
br i1 %4, label %5, label %7
5: ; preds = %1
%6 = call i32 (ptr, ...) @printf(ptr noundef nonnull dereferenceable(1) @.str, i32 noundef %2)
ret i32 0
7: ; preds = %1
%8 = icmp samesign ugt i32 %2, 67108864
br i1 %8, label %9, label %10
9: ; preds = %7
call void @exit(i32 noundef 13) #3
unreachable
10: ; preds = %7
%11 = add nuw nsw i32 %2, 1
%12 = fadd float %3, 1.000000e+00
br label %1, !llvm.loop !9
}
declare void @exit(i32 noundef) local_unnamed_addr
declare noundef i32 @printf(ptr noundef readonly captures(none), ...)
!9 = distinct !{!9, !10}
!10 = !{!"llvm.loop.mustprogress"}
After opt -O3 with your patch:
define dso_local noundef i32 @main() local_unnamed_addr #0 {
%1 = tail call i32 (ptr, ...) @printf(ptr noundef nonnull dereferenceable(1) @.str, i32 noundef 16777217)
ret i32 0
}
d43a6c7 to
f60d26d
Compare
Thanks, good catch. While indeed integers from |
When transforming floating-point induction variables into integer ones, make sure we stay within the bounds of fp values that can be represented as integers without gaps, i.e., 2^24 and 2^53 for IEEE-754 single and double precision respectively (both on negative and positive side). Fixes: llvm#166496.
antoniofrighetto
force-pushed
the
feature/indvars-fps-to-representable-int
branch
from
f60d26d to
eaf3a91
Compare
5 participants
When transforming floating-point induction variables into integer ones, make sure we stay within the bounds of fp values that can be represented as integers without gaps, i.e., 2^24 and 2^53 for IEEE-754 single and double precision respectively (both on negative and positive side).
Fixes: #166496.