[llvm] 248fcab - [InstCombine] Do not use operand info in `replaceInInstruction` (#99492)
via llvm-commits
llvm-commits at lists.llvm.org
Sun Jul 21 20:59:58 PDT 2024
Author: Yingwei Zheng
Date: 2024-07-22T11:59:54+08:00
New Revision: 248fcab2fc9b3fc1bde5cd5b1fe8615791225c9e
URL: https://github.com/llvm/llvm-project/commit/248fcab2fc9b3fc1bde5cd5b1fe8615791225c9e
DIFF: https://github.com/llvm/llvm-project/commit/248fcab2fc9b3fc1bde5cd5b1fe8615791225c9e.diff
LOG: [InstCombine] Do not use operand info in `replaceInInstruction` (#99492)
Consider the following case:
```
%cmp = icmp eq ptr %p, null
%load = load i32, ptr %p, align 4
%sel = select i1 %cmp, i32 %load, i32 0
```
`foldSelectValueEquivalence` converts `load i32, ptr %p, align 4` into
`load i32, ptr null, align 4`, which causes immediate UB. `%load` is
speculatable, but it doesn't hold after operand substitution.
This patch introduces a new helper
`isSafeToSpeculativelyExecuteWithVariableReplaced`. It ignores operand
info in these instructions since their operands will be replaced later.
Fixes #99436.
---------
Co-authored-by: Nikita Popov <github at npopov.com>
Added:
Modified:
llvm/include/llvm/Analysis/ValueTracking.h
llvm/lib/Analysis/ValueTracking.cpp
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
llvm/test/Transforms/InstCombine/select-binop-cmp.ll
llvm/test/Transforms/InstCombine/select.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 2c2f965a3cd6f..44e27234aa90d 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -822,15 +822,25 @@ bool isSafeToSpeculativelyExecute(const Instruction *I,
const Instruction *CtxI = nullptr,
AssumptionCache *AC = nullptr,
const DominatorTree *DT = nullptr,
- const TargetLibraryInfo *TLI = nullptr);
+ const TargetLibraryInfo *TLI = nullptr,
+ bool UseVariableInfo = true);
+
+inline bool isSafeToSpeculativelyExecute(const Instruction *I,
+ BasicBlock::iterator CtxI,
+ AssumptionCache *AC = nullptr,
+ const DominatorTree *DT = nullptr,
+ const TargetLibraryInfo *TLI = nullptr,
+ bool UseVariableInfo = true) {
+ // Take an iterator, and unwrap it into an Instruction *.
+ return isSafeToSpeculativelyExecute(I, &*CtxI, AC, DT, TLI, UseVariableInfo);
+}
+/// Don't use information from its non-constant operands. This helper is used
+/// when its operands are going to be replaced.
inline bool
-isSafeToSpeculativelyExecute(const Instruction *I, BasicBlock::iterator CtxI,
- AssumptionCache *AC = nullptr,
- const DominatorTree *DT = nullptr,
- const TargetLibraryInfo *TLI = nullptr) {
- // Take an iterator, and unwrap it into an Instruction *.
- return isSafeToSpeculativelyExecute(I, &*CtxI, AC, DT, TLI);
+isSafeToSpeculativelyExecuteWithVariableReplaced(const Instruction *I) {
+ return isSafeToSpeculativelyExecute(I, nullptr, nullptr, nullptr, nullptr,
+ /*UseVariableInfo=*/false);
}
/// This returns the same result as isSafeToSpeculativelyExecute if Opcode is
@@ -853,7 +863,7 @@ isSafeToSpeculativelyExecute(const Instruction *I, BasicBlock::iterator CtxI,
bool isSafeToSpeculativelyExecuteWithOpcode(
unsigned Opcode, const Instruction *Inst, const Instruction *CtxI = nullptr,
AssumptionCache *AC = nullptr, const DominatorTree *DT = nullptr,
- const TargetLibraryInfo *TLI = nullptr);
+ const TargetLibraryInfo *TLI = nullptr, bool UseVariableInfo = true);
/// Returns true if the result or effects of the given instructions \p I
/// depend values not reachable through the def use graph.
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 03eb6ef42b0ff..5c56654246ead 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -6771,15 +6771,16 @@ bool llvm::isSafeToSpeculativelyExecute(const Instruction *Inst,
const Instruction *CtxI,
AssumptionCache *AC,
const DominatorTree *DT,
- const TargetLibraryInfo *TLI) {
+ const TargetLibraryInfo *TLI,
+ bool UseVariableInfo) {
return isSafeToSpeculativelyExecuteWithOpcode(Inst->getOpcode(), Inst, CtxI,
- AC, DT, TLI);
+ AC, DT, TLI, UseVariableInfo);
}
bool llvm::isSafeToSpeculativelyExecuteWithOpcode(
unsigned Opcode, const Instruction *Inst, const Instruction *CtxI,
- AssumptionCache *AC, const DominatorTree *DT,
- const TargetLibraryInfo *TLI) {
+ AssumptionCache *AC, const DominatorTree *DT, const TargetLibraryInfo *TLI,
+ bool UseVariableInfo) {
#ifndef NDEBUG
if (Inst->getOpcode() != Opcode) {
// Check that the operands are actually compatible with the Opcode override.
@@ -6831,6 +6832,9 @@ bool llvm::isSafeToSpeculativelyExecuteWithOpcode(
return false;
}
case Instruction::Load: {
+ if (!UseVariableInfo)
+ return false;
+
const LoadInst *LI = dyn_cast<LoadInst>(Inst);
if (!LI)
return false;
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index e387034110df9..aaf4ece3249a2 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1247,8 +1247,11 @@ bool InstCombinerImpl::replaceInInstruction(Value *V, Value *Old, Value *New,
if (Depth == 2)
return false;
+ assert(!isa<Constant>(Old) && "Only replace non-constant values");
+
auto *I = dyn_cast<Instruction>(V);
- if (!I || !I->hasOneUse() || !isSafeToSpeculativelyExecute(I))
+ if (!I || !I->hasOneUse() ||
+ !isSafeToSpeculativelyExecuteWithVariableReplaced(I))
return false;
bool Changed = false;
@@ -1331,7 +1334,7 @@ Instruction *InstCombinerImpl::foldSelectValueEquivalence(SelectInst &Sel,
// profitability is not clear for other cases.
// FIXME: Support vectors.
if (OldOp == CmpLHS && match(NewOp, m_ImmConstant()) &&
- !match(OldOp, m_ImmConstant()) && !Cmp.getType()->isVectorTy() &&
+ !match(OldOp, m_Constant()) && !Cmp.getType()->isVectorTy() &&
isGuaranteedNotToBeUndef(NewOp, SQ.AC, &Sel, &DT))
if (replaceInInstruction(TrueVal, OldOp, NewOp))
return &Sel;
diff --git a/llvm/test/Transforms/InstCombine/select-binop-cmp.ll b/llvm/test/Transforms/InstCombine/select-binop-cmp.ll
index 1fa0c09a9e987..fb56764598e2d 100644
--- a/llvm/test/Transforms/InstCombine/select-binop-cmp.ll
+++ b/llvm/test/Transforms/InstCombine/select-binop-cmp.ll
@@ -1315,6 +1315,19 @@ define i32 @select_replace_call_speculatable(i32 %x, i32 %y) {
ret i32 %s
}
+define i32 @select_replace_call_speculatable_intrinsic(i32 %x, i32 %y) {
+; CHECK-LABEL: @select_replace_call_speculatable_intrinsic(
+; CHECK-NEXT: [[C:%.*]] = icmp eq i32 [[X:%.*]], 0
+; CHECK-NEXT: [[CALL:%.*]] = call i32 @llvm.smax.i32(i32 [[Y:%.*]], i32 0)
+; CHECK-NEXT: [[S:%.*]] = select i1 [[C]], i32 [[CALL]], i32 [[Y]]
+; CHECK-NEXT: ret i32 [[S]]
+;
+ %c = icmp eq i32 %x, 0
+ %call = call i32 @llvm.smax.i32(i32 %x, i32 %y)
+ %s = select i1 %c, i32 %call, i32 %y
+ ret i32 %s
+}
+
; We can't replace the call arguments, as the call is not speculatable. We
; may end up changing side-effects or causing undefined behavior.
define i32 @select_replace_call_non_speculatable(i32 %x, i32 %y) {
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index d66ffb9a63ac1..1369be305ec13 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -4713,3 +4713,19 @@ define i8 @select_knownbits_simplify_missing_noundef(i8 %x) {
%res = select i1 %cmp, i8 %and, i8 0
ret i8 %res
}
+
+ at g_ext = external global i8
+
+; Make sure we don't replace %ptr with @g_ext, which may cause the load to trigger UB.
+define i32 @pr99436(ptr align 4 dereferenceable(4) %ptr) {
+; CHECK-LABEL: @pr99436(
+; CHECK-NEXT: [[CMP:%.*]] = icmp eq ptr [[PTR:%.*]], @g_ext
+; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr [[PTR]], align 4
+; CHECK-NEXT: [[RET:%.*]] = select i1 [[CMP]], i32 [[VAL]], i32 0
+; CHECK-NEXT: ret i32 [[RET]]
+;
+ %cmp = icmp eq ptr %ptr, @g_ext
+ %val = load i32, ptr %ptr, align 4
+ %ret = select i1 %cmp, i32 %val, i32 0
+ ret i32 %ret
+}
More information about the llvm-commits
mailing list