[ConstantRange] Fix off by 1 bugs in UIToFP and SIToFP handling. #86041
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Craig Topper (topperc) ChangesWe were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason #79158 was reverted. CC: @AtariDreams Full diff: https://github.com/llvm/llvm-project/pull/86041.diff 2 Files Affected:
diff --git a/llvm/lib/IR/ConstantRange.cpp b/llvm/lib/IR/ConstantRange.cpp
index 3394a1ec8dc476..59e7a9f5eb1114 100644
--- a/llvm/lib/IR/ConstantRange.cpp
+++ b/llvm/lib/IR/ConstantRange.cpp
@@ -746,7 +746,7 @@ ConstantRange ConstantRange::castOp(Instruction::CastOps CastOp,
Min = Min.zext(ResultBitWidth);
Max = Max.zext(ResultBitWidth);
}
- return ConstantRange(std::move(Min), std::move(Max));
+ return getNonEmpty(std::move(Min), std::move(Max) + 1);
}
case Instruction::SIToFP: {
// TODO: use input range if available
@@ -757,7 +757,7 @@ ConstantRange ConstantRange::castOp(Instruction::CastOps CastOp,
SMin = SMin.sext(ResultBitWidth);
SMax = SMax.sext(ResultBitWidth);
}
- return ConstantRange(std::move(SMin), std::move(SMax));
+ return getNonEmpty(std::move(SMin), std::move(SMax) + 1);
}
case Instruction::FPTrunc:
case Instruction::FPExt:
diff --git a/llvm/unittests/IR/ConstantRangeTest.cpp b/llvm/unittests/IR/ConstantRangeTest.cpp
index 34a162a5514e95..8ec120d70e99f6 100644
--- a/llvm/unittests/IR/ConstantRangeTest.cpp
+++ b/llvm/unittests/IR/ConstantRangeTest.cpp
@@ -2479,6 +2479,24 @@ TEST_F(ConstantRangeTest, castOps) {
ConstantRange IntToPtr = A.castOp(Instruction::IntToPtr, 64);
EXPECT_EQ(64u, IntToPtr.getBitWidth());
EXPECT_TRUE(IntToPtr.isFullSet());
+
+ ConstantRange UIToFP = A.castOp(Instruction::UIToFP, 16);
+ EXPECT_EQ(16u, UIToFP.getBitWidth());
+ EXPECT_TRUE(UIToFP.isFullSet());
+
+ ConstantRange UIToFP2 = A.castOp(Instruction::UIToFP, 64);
+ ConstantRange B(APInt(64, 0), APInt(64, 65536));
+ EXPECT_EQ(64u, UIToFP2.getBitWidth());
+ EXPECT_EQ(B, UIToFP2);
+
+ ConstantRange SIToFP = A.castOp(Instruction::SIToFP, 16);
+ EXPECT_EQ(16u, SIToFP.getBitWidth());
+ EXPECT_TRUE(SIToFP.isFullSet());
+
+ ConstantRange SIToFP2 = A.castOp(Instruction::SIToFP, 64);
+ ConstantRange C(APInt(64, -32768), APInt(64, 32768));
+ EXPECT_EQ(64u, SIToFP2.getBitWidth());
+ EXPECT_EQ(C, SIToFP2);
}
TEST_F(ConstantRangeTest, binaryAnd) {
|
RSilicon
approved these changes
Mar 20, 2024
Could you please add the float2int tests while you are at it? |
topperc
force-pushed
the
pr/constant-range-itofp
branch
from
3054d11
to
e3c5053
Compare
nikic
approved these changes
Mar 21, 2024
| ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4 | ||
| ; RUN: opt < %s -passes=float2int -S | FileCheck %s | ||
|
|
||
| define i32 @pr79158(i32 %x.i.0.x.i.0.x.0.x.0.x.0..i) { |
There was a problem hiding this comment.
Please adjust the variable name :)
topperc
force-pushed
the
pr/constant-range-itofp
branch
from
e3c5053
to
7a62d5f
Compare
We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted.
topperc
force-pushed
the
pr/constant-range-itofp
branch
from
7a62d5f
to
83bb4a3
Compare
Error: Command failed due to missing milestone. |
Error: Command failed due to missing milestone. |
Error: Command failed due to missing milestone. |
Error: Command failed due to missing milestone. |
Error: Command failed due to missing milestone. |
|
Failed to cherry-pick: release/18.x https://github.com/llvm/llvm-project/actions/runs/8378684757 Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request |
|
/pull-request #86153 |
llvmbot
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Mar 21, 2024
…m#86041) We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted. (cherry picked from commit 1283646)
RSilicon
pushed a commit
to RSilicon/llvm-project
that referenced
this pull request
Mar 21, 2024
…m#86041) We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted.
RSilicon
pushed a commit
to RSilicon/llvm-project
that referenced
this pull request
Mar 21, 2024
…m#86041) We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted.
chencha3
pushed a commit
to chencha3/llvm-project
that referenced
this pull request
Mar 23, 2024
…m#86041) We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted.
tstellar
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Apr 10, 2024
…m#86041) We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted. (cherry picked from commit 1283646)
tstellar
pushed a commit
to llvmbot/llvm-project
that referenced
this pull request
Apr 10, 2024
…m#86041) We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1. We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion. I believe this fixes at least some of the reason llvm#79158 was reverted. (cherry picked from commit 1283646)
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were passing the min and max values of the range to the ConstantRange constructor, but the constructor expects the upper bound to 1 more than the max value so we need to add 1.
We also need to use getNonEmpty so that passing 0, 0 to the constructor creates a full range rather than an empty range. And passing smin, smax+1 doesn't cause an assertion.
I believe this fixes at least some of the reason #79158 was reverted.
CC: @AtariDreams