[ConstantRange] Fix off by 1 bugs in UIToFP and SIToFP handling. #86041
Conversation
This file contains hidden or 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
Member
| @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) { |
AZero13 approved these changes Mar 20, 2024
Contributor
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) { |
Contributor
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 Member
Error: Command failed due to missing milestone. |
Member
Error: Command failed due to missing milestone. |
Member
Error: Command failed due to missing milestone. |
Member
Error: Command failed due to missing milestone. |
Member
Error: Command failed due to missing milestone. |
Contributor
Member
| 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 |
Member
| /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)
AZero13 pushed a commit to AZero13/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.
AZero13 pushed a commit to AZero13/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
Tedlion pushed a commit to Tedlion/llvm-project that referenced this pull request Jun 15, 2025
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
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