Skip to content

Update SimplifyIndVar.cpp #69760

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.

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

Merged
merged 1 commit into from
Oct 20, 2023
Merged

Conversation

andykaylor
Copy link
Contributor

In SimplifyIndvar::replaceFloatIVWithIntegerIV() the return value of getFPMantissaWidth() was being cast as an unsigned integer and then compared with the number of bits needed to represent an integer that was cast to and from a floating-point type. This is a problem because getFPMantissaWidth() returns -1 if the type does not have a stable mantissa.

Currently the only type that returns -1 is ppc_fp128, so you'd need a pretty big induction variable to cause a problem. However, this problem will be more likely to be exposed when we implement support for decimal floating-point types.

Strictly speaking, what we want to know here is the size of the biggest integer that can be represented exactly. We could get that information even with an unstable mantissa width, but getFPMantissaWidth() won't do it.

In SimplifyIndvar::replaceFloatIVWithIntegerIV() the return value of getFPMantissaWidth() was being cast as an unsigned integer and then compared with the number of bits needed to represent an integer that was cast to and from a floating-point type. This is a problem because getFPMantissaWidth() returns -1 if the type does not have a stable mantissa.

Currently the only type that returns -1 is ppc_fp128, so you'd need a pretty big induction variable to cause a problem. However, this problem will be more likely to be exposed when we implement support for decimal floating-point types.

Strictly speaking, what we want to know here is the size of the biggest integer that can be represented exactly. We could get that information even with an unstable mantissa width, but getFPMantissaWidth() won't do it.
@llvmbot
Copy link
Member

llvmbot commented Oct 20, 2023

@llvm/pr-subscribers-llvm-transforms

Author: Andy Kaylor (andykaylor)

Changes

In SimplifyIndvar::replaceFloatIVWithIntegerIV() the return value of getFPMantissaWidth() was being cast as an unsigned integer and then compared with the number of bits needed to represent an integer that was cast to and from a floating-point type. This is a problem because getFPMantissaWidth() returns -1 if the type does not have a stable mantissa.

Currently the only type that returns -1 is ppc_fp128, so you'd need a pretty big induction variable to cause a problem. However, this problem will be more likely to be exposed when we implement support for decimal floating-point types.

Strictly speaking, what we want to know here is the size of the biggest integer that can be represented exactly. We could get that information even with an unstable mantissa width, but getFPMantissaWidth() won't do it.


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

1 Files Affected:

  • (modified) llvm/lib/Transforms/Utils/SimplifyIndVar.cpp (+1-1)
diff --git a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
index 1caf708bcc35254..45cbdd235898b28 100644
--- a/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
@@ -664,7 +664,7 @@ bool SimplifyIndvar::replaceFloatIVWithIntegerIV(Instruction *UseInst) {
     MaskBits = SE->getSignedRange(IV).getMinSignedBits();
   else
     MaskBits = SE->getUnsignedRange(IV).getActiveBits();
-  unsigned DestNumSigBits = UseInst->getType()->getFPMantissaWidth();
+  int DestNumSigBits = UseInst->getType()->getFPMantissaWidth();
   if (MaskBits <= DestNumSigBits) {
     for (User *U : UseInst->users()) {
       // Match for fptosi/fptoui of sitofp and with same type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants