[ValueTracking] Bail out on non-immediate constant expressions #168084
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
llvmbot
added
llvm:analysis
Member
|
@llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-transforms Author: Yingwei Zheng (dtcxzyw) ChangesIn #165748 constant expressions were allowed in Fix the miscompilation reported by #165748 (comment) Full diff: https://github.com/llvm/llvm-project/pull/168084.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 093309cb8bbee..b730a36488780 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -1024,8 +1024,8 @@ findValuesAffectedByCondition(Value *Cond, bool IsAssume,
LLVM_ABI Value *stripNullTest(Value *V);
LLVM_ABI const Value *stripNullTest(const Value *V);
-/// Enumerates all possible values of V and inserts them into the set \p
-/// Constants. If \p AllowUndefOrPoison is false, it fails when V may contain
+/// Enumerates all possible immediate values of V and inserts them into the set
+/// \p Constants. If \p AllowUndefOrPoison is false, it fails when V may contain
/// undef/poison elements. Returns true if the result is complete. Otherwise,
/// the result is incomplete (more than MaxCount values).
/// NOTE: The constant values are not distinct.
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 41ff816a33262..b20542c65f491 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -10485,7 +10485,8 @@ bool llvm::collectPossibleValues(const Value *V,
SmallPtrSet<const Instruction *, 8> Visited;
SmallVector<const Instruction *, 8> Worklist;
auto Push = [&](const Value *V) -> bool {
- if (auto *C = dyn_cast<Constant>(V)) {
+ Constant *C;
+ if (match(const_cast<Value *>(V), m_ImmConstant(C))) {
if (!AllowUndefOrPoison && !isGuaranteedNotToBeUndefOrPoison(C))
return false;
// Check existence first to avoid unnecessary allocations.
diff --git a/llvm/test/Transforms/SimplifyCFG/switch-on-const.ll b/llvm/test/Transforms/SimplifyCFG/switch-on-const.ll
index 1ab1b5e8bd838..541bdf5ef996e 100644
--- a/llvm/test/Transforms/SimplifyCFG/switch-on-const.ll
+++ b/llvm/test/Transforms/SimplifyCFG/switch-on-const.ll
@@ -280,6 +280,36 @@ default:
ret void
}
+@G = constant i16 zeroinitializer, align 1
+
+; Make sure we don't remove edges when the condition is a unresolved constant expression.
+
+define i16 @switch_on_nonimmediate_constant_expr() {
+; CHECK-LABEL: @switch_on_nonimmediate_constant_expr(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[SWITCH_SELECTCMP:%.*]] = icmp eq i32 ptrtoint (ptr @G to i32), 2
+; CHECK-NEXT: [[SWITCH_SELECT:%.*]] = select i1 [[SWITCH_SELECTCMP]], i16 456, i16 789
+; CHECK-NEXT: [[SWITCH_SELECTCMP1:%.*]] = icmp eq i32 ptrtoint (ptr @G to i32), 1
+; CHECK-NEXT: [[SWITCH_SELECT2:%.*]] = select i1 [[SWITCH_SELECTCMP1]], i16 123, i16 [[SWITCH_SELECT]]
+; CHECK-NEXT: ret i16 [[SWITCH_SELECT2]]
+;
+entry:
+ switch i32 ptrtoint (ptr @G to i32), label %sw.default [
+ i32 1, label %sw.bb
+ i32 2, label %sw.bb1
+ ]
+
+sw.bb:
+ ret i16 123
+
+sw.bb1:
+ ret i16 456
+
+sw.default:
+ ret i16 789
+}
+
+
declare void @llvm.trap() nounwind noreturn
declare void @bees.a() nounwind
declare void @bees.b() nounwind
|
Collaborator
|
I verified that it also fixes my original reproducer. (and not only the reduced version I posted in the original PR) |
dianqk
approved these changes
Nov 15, 2025
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/17095 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
In #165748 constant expressions were allowed in
collectPossibleValuesbecause we are still using insertelement + shufflevector idioms to represent a scalable vector splat. However, it also accepts some unresolved constants like ptrtoint of globals or pointer difference between two globals. Absolutely we can ask the user to check this case with the constant folding API. However, since we don't observe the real-world usefulness of handling constant expressions, I decide to be more conservative and only handle immediate constants in the helper function. With this patch, we don't need to touch the SimplifyCFG part, as the values can only be either ConstantInt or undef/poison values (NB: switch on undef condition is UB).Fix the miscompilation reported by #165748 (comment)