-
Notifications
You must be signed in to change notification settings - Fork 10.8k
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
[ValueTracking] Fix KnownBits conflict for calls (range vs returned) #84353
Conversation
If a function only exits for certain input values we can still derive that an argument is "returned". We can also derive range metadata that describe the possible value range returned by the function. However, it turns out that those two analyses can result in conflicting information. Example: declare i16 @foo(i16 returned) ... %A = call i16 @foo(i16 4095), !range !{i16 32, i16 33} To avoid "Bits known to be one AND zero?" assertion failures we know make sure to discard the known bits for this kind of scenario.
@llvm/pr-subscribers-llvm-analysis Author: Björn Pettersson (bjope) ChangesIf a function only exits for certain input values we can still derive that an argument is "returned". We can also derive range metadata that describe the possible value range returned by the function. However, it turns out that those two analyses can result in conflicting information. Example: To avoid "Bits known to be one AND zero?" assertion failures we know make sure to discard the known bits for this kind of scenario. Full diff: https://github.com/llvm/llvm-project/pull/84353.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 52ae9f034e5d34..6d0e79e11eed43 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1476,6 +1476,12 @@ static void computeKnownBitsFromOperator(const Operator *I,
if (RV->getType() == I->getType()) {
computeKnownBits(RV, Known2, Depth + 1, Q);
Known = Known.unionWith(Known2);
+ // If the function doesn't return properly for all input values
+ // (e.g. unreachable exits) then there might be conflicts between the
+ // argument value and the range metadata. Simply discard the known bits
+ // in case of conflicts.
+ if (Known.hasConflict())
+ Known.resetAll();
}
}
if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index 9e0abe7a16df98..6c6897d83a256e 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -2359,6 +2359,20 @@ TEST_F(ComputeKnownBitsTest, ComputeKnownBitsFreeze) {
EXPECT_EQ(Known.One.getZExtValue(), 0u);
}
+TEST_F(ComputeKnownBitsTest, ComputeKnownBitsReturnedRangeConflict) {
+ parseAssembly(
+ "declare i16 @foo(i16 returned)\n"
+ "\n"
+ "define i16 @test() {\n"
+ " %A = call i16 @foo(i16 4095), !range !{i16 32, i16 33}\n"
+ " ret i16 %A\n"
+ "}\n");
+ // The call returns 32 according to range metadata, but 4095 according to the
+ // returned arg operand. Given the conflicting information we expect that the
+ // known bits information simply is cleared.
+ expectKnownBits(/*zero*/ 0u, /*one*/ 0u);
+}
+
TEST_F(ComputeKnownBitsTest, ComputeKnownBitsAddWithRange) {
parseAssembly("define void @test(ptr %p) {\n"
" %A = load i64, ptr %p, !range !{i64 64, i64 65536}\n"
|
You can test this locally with the following command:git-clang-format --diff 3714f937b835c06c8c32ca4f3f61ba2317db2296 bdbf290ed97417a45feafe3a73dc32c1885ed4a5 -- llvm/lib/Analysis/ValueTracking.cpp llvm/unittests/Analysis/ValueTrackingTest.cpp View the diff from clang-format here.diff --git a/llvm/unittests/Analysis/ValueTrackingTest.cpp b/llvm/unittests/Analysis/ValueTrackingTest.cpp
index 6c6897d83a..0a79e09196 100644
--- a/llvm/unittests/Analysis/ValueTrackingTest.cpp
+++ b/llvm/unittests/Analysis/ValueTrackingTest.cpp
@@ -2360,13 +2360,12 @@ TEST_F(ComputeKnownBitsTest, ComputeKnownBitsFreeze) {
}
TEST_F(ComputeKnownBitsTest, ComputeKnownBitsReturnedRangeConflict) {
- parseAssembly(
- "declare i16 @foo(i16 returned)\n"
- "\n"
- "define i16 @test() {\n"
- " %A = call i16 @foo(i16 4095), !range !{i16 32, i16 33}\n"
- " ret i16 %A\n"
- "}\n");
+ parseAssembly("declare i16 @foo(i16 returned)\n"
+ "\n"
+ "define i16 @test() {\n"
+ " %A = call i16 @foo(i16 4095), !range !{i16 32, i16 33}\n"
+ " ret i16 %A\n"
+ "}\n");
// The call returns 32 according to range metadata, but 4095 according to the
// returned arg operand. Given the conflicting information we expect that the
// known bits information simply is cleared.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. We should really get rid of these conflict assertions one day.
@@ -1476,6 +1476,12 @@ static void computeKnownBitsFromOperator(const Operator *I, | |||
if (RV->getType() == I->getType()) { | |||
computeKnownBits(RV, Known2, Depth + 1, Q); | |||
Known = Known.unionWith(Known2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this not be just intersectWith?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was a bit confused as well given that the code comments talks about intersect, and then the code is doing union.
But I figured that generally we could have that the range matadata says that the function returns values in the range [0, 15] while the returned arg is known to be 8. Given that we use unionWith we would derive that the call will return 8. If we use intersectWith, then we would only get the common knowledge of bits that are zero/one. So the know bits would indicate that the returned value is in the range [0, 15].
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The naming of the methods is a bit confusing. This is a union in the sense that we take all the available information from both operands. Not in the sense that we union two ranges.
I guess that unionWith is a usual culprit of creating KnownBits that has conflicts. One idea could be to avoid hasConflict by letting unionWith reset the info by itself in case the result would be conflicting. |
Long-term we'd like to just keep the conflict result and fold to poison based on it. Just annoying to carry that through. But short-term, doing the reset directly in unionWith does sound like a good way to avoid most of these issues... |
If a function only exits for certain input values we can still derive that an argument is "returned". We can also derive range metadata that describe the possible value range returned by the function. However, it turns out that those two analyses can result in conflicting information.
Example:
declare i16 @foo(i16 returned)
...
%A = call i16 @foo(i16 4095), !range !{i16 32, i16 33}
To avoid "Bits known to be one AND zero?" assertion failures we know make sure to discard the known bits for this kind of scenario.