[InstCombine] Do not use operand info in `replaceInInstruction` #99492

dtcxzyw · 2024-07-18T13:38:11Z

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.

llvmbot · 2024-07-18T13:38:42Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

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 isSafeToSpeculativelyExecuteWithoutInstrInfo. It ignores operand info in these instructions since their operands will be replaced later.

Fixes #99436.

Full diff: https://github.com/llvm/llvm-project/pull/99492.diff

4 Files Affected:

(modified) llvm/include/llvm/Analysis/ValueTracking.h (+18-9)
(modified) llvm/lib/Analysis/ValueTracking.cpp (+13-9)
(modified) llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp (+10-1)
(modified) llvm/test/Transforms/InstCombine/select.ll (+22-1)

diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h
index 2c2f965a3cd6f..0cad7943a98a4 100644
--- a/llvm/include/llvm/Analysis/ValueTracking.h
+++ b/llvm/include/llvm/Analysis/ValueTracking.h
@@ -822,15 +822,24 @@ bool isSafeToSpeculativelyExecute(const Instruction *I,
                                   const Instruction *CtxI = nullptr,
                                   AssumptionCache *AC = nullptr,
                                   const DominatorTree *DT = nullptr,
-                                  const TargetLibraryInfo *TLI = nullptr);
-
-inline bool
-isSafeToSpeculativelyExecute(const Instruction *I, BasicBlock::iterator CtxI,
-                             AssumptionCache *AC = nullptr,
-                             const DominatorTree *DT = nullptr,
-                             const TargetLibraryInfo *TLI = nullptr) {
+                                  const TargetLibraryInfo *TLI = nullptr,
+                                  bool UseInstrInfo = true);
+
+inline bool isSafeToSpeculativelyExecute(const Instruction *I,
+                                         BasicBlock::iterator CtxI,
+                                         AssumptionCache *AC = nullptr,
+                                         const DominatorTree *DT = nullptr,
+                                         const TargetLibraryInfo *TLI = nullptr,
+                                         bool UseInstrInfo = true) {
   // Take an iterator, and unwrap it into an Instruction *.
-  return isSafeToSpeculativelyExecute(I, &*CtxI, AC, DT, TLI);
+  return isSafeToSpeculativelyExecute(I, &*CtxI, AC, DT, TLI, UseInstrInfo);
+}
+
+/// Don't use information from its operands. This helper is used when its
+/// operands are going to be replaced.
+inline bool isSafeToSpeculativelyExecuteWithoutInstrInfo(const Instruction *I) {
+  return isSafeToSpeculativelyExecute(I, nullptr, nullptr, nullptr, nullptr,
+                                      /*UseInstrInfo=*/false);
 }
 
 /// This returns the same result as isSafeToSpeculativelyExecute if Opcode is
@@ -853,7 +862,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 UseInstrInfo = 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 535a248a5f1a2..e84622755c050 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -6756,19 +6756,17 @@ bool llvm::mustSuppressSpeculation(const LoadInst &LI) {
     F.hasFnAttribute(Attribute::SanitizeHWAddress);
 }
 
-bool llvm::isSafeToSpeculativelyExecute(const Instruction *Inst,
-                                        const Instruction *CtxI,
-                                        AssumptionCache *AC,
-                                        const DominatorTree *DT,
-                                        const TargetLibraryInfo *TLI) {
+bool llvm::isSafeToSpeculativelyExecute(
+    const Instruction *Inst, const Instruction *CtxI, AssumptionCache *AC,
+    const DominatorTree *DT, const TargetLibraryInfo *TLI, bool UseInstrInfo) {
   return isSafeToSpeculativelyExecuteWithOpcode(Inst->getOpcode(), Inst, CtxI,
-                                                AC, DT, TLI);
+                                                AC, DT, TLI, UseInstrInfo);
 }
 
 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 UseInstrInfo) {
 #ifndef NDEBUG
   if (Inst->getOpcode() != Opcode) {
     // Check that the operands are actually compatible with the Opcode override.
@@ -6796,12 +6794,15 @@ bool llvm::isSafeToSpeculativelyExecuteWithOpcode(
   case Instruction::URem: {
     // x / y is undefined if y == 0.
     const APInt *V;
-    if (match(Inst->getOperand(1), m_APInt(V)))
+    if (UseInstrInfo && match(Inst->getOperand(1), m_APInt(V)))
       return *V != 0;
     return false;
   }
   case Instruction::SDiv:
   case Instruction::SRem: {
+    if (!UseInstrInfo)
+      return false;
+
     // x / y is undefined if y == 0 or x == INT_MIN and y == -1
     const APInt *Numerator, *Denominator;
     if (!match(Inst->getOperand(1), m_APInt(Denominator)))
@@ -6820,6 +6821,9 @@ bool llvm::isSafeToSpeculativelyExecuteWithOpcode(
     return false;
   }
   case Instruction::Load: {
+    if (!UseInstrInfo)
+      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..edcd9c9f29ddc 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -1248,9 +1248,18 @@ bool InstCombinerImpl::replaceInInstruction(Value *V, Value *Old, Value *New,
     return false;
 
   auto *I = dyn_cast<Instruction>(V);
-  if (!I || !I->hasOneUse() || !isSafeToSpeculativelyExecute(I))
+  if (!I || !I->hasOneUse() || !isSafeToSpeculativelyExecuteWithoutInstrInfo(I))
     return false;
 
+  // Special handling for replacing called operand
+  if (auto *Call = dyn_cast<CallInst>(I)) {
+    if (Call->getCalledOperand() == Old) {
+      auto *Callee = dyn_cast<Function>(New);
+      if (!Callee || !Callee->isSpeculatable())
+        return false;
+    }
+  }
+
   bool Changed = false;
   for (Use &U : I->operands()) {
     if (U == Old) {
diff --git a/llvm/test/Transforms/InstCombine/select.ll b/llvm/test/Transforms/InstCombine/select.ll
index d66ffb9a63ac1..88a18360645a8 100644
--- a/llvm/test/Transforms/InstCombine/select.ll
+++ b/llvm/test/Transforms/InstCombine/select.ll
@@ -1,5 +1,5 @@
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt < %s -passes=instcombine -S | FileCheck %s
+; RUN: opt < %s -passes='instcombine<no-verify-fixpoint>' -S | FileCheck %s
 
 ; PR1822
 
@@ -4713,3 +4713,24 @@ define i8 @select_knownbits_simplify_missing_noundef(i8 %x)  {
   %res = select i1 %cmp, i8 %and, i8 0
   ret i8 %res
 }
+
+@arr = global [2 x i32] zeroinitializer, align 4
+@cst = constant ptr getelementptr (i8, ptr @arr, i64 4)
+
+define i32 @pr99436() {
+; CHECK-LABEL: @pr99436(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq ptr getelementptr (i8, ptr @arr, i64 4), null
+; CHECK-NEXT:    [[VAL:%.*]] = load i32, ptr getelementptr (i8, ptr @arr, i64 4), align 4
+; CHECK-NEXT:    [[RET:%.*]] = select i1 [[CMP]], i32 [[VAL]], i32 0
+; CHECK-NEXT:    ret i32 [[RET]]
+;
+entry:
+  %alloc = alloca ptr, align 8
+  call void @llvm.memcpy.p0.p0.i64(ptr align 8 %alloc, ptr align 8 @cst, i64 8, i1 false)
+  %ptr = load ptr, ptr %alloc, align 8
+  %cmp = icmp eq ptr %ptr, null
+  %val = load i32, ptr %ptr, align 4
+  %ret = select i1 %cmp, i32 %val, i32 0
+  ret i32 %ret
+}

llvm/test/Transforms/InstCombine/select.ll

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

llvm/test/Transforms/InstCombine/select.ll

nikic

I think we already discovered this issue as part of @goldsteinn's attempt to support non-constants in div speculation, but never followed up on it. Oops.

llvm/include/llvm/Analysis/ValueTracking.h

llvm/test/Transforms/InstCombine/select.ll

antoniofrighetto · 2024-07-18T13:46:31Z

How was this miscompilation found, out of curiosity?

dtcxzyw · 2024-07-18T14:00:14Z

How was this miscompilation found, out of curiosity?

This bug was found by csmith.

llvm/include/llvm/Analysis/ValueTracking.h

nikic · 2024-07-18T14:38:35Z

llvm/lib/Analysis/ValueTracking.cpp

@@ -6796,12 +6797,15 @@ bool llvm::isSafeToSpeculativelyExecuteWithOpcode(
  case Instruction::URem: {
    // x / y is undefined if y == 0.
    const APInt *V;
-    if (match(Inst->getOperand(1), m_APInt(V)))
+    if (UseOperandInfo && match(Inst->getOperand(1), m_APInt(V)))


I'm wondering whether it may make more sense to say that isSafeToSpeculativelyExecuteWithOperandsReplaced only allows replacement of non-constant operands, which I think it all we ever care about. In that case the division cases can be kept and we also don't have to worry about the call case specially (as it already only does something for constant callee).

While I think defacto this would be fine, the usage that broke my patch way back when was truncation of division operands which did modify the ~~non-const~~ const operands.

Hm, good point on the truncation.

My concern here is that we have quite a few transforms currently using isSafeToSpeculativelyExecute() when they're really interested in operand replacements, and most of these transforms are specifically interested in div/rem. It would be great if we could replace those uses of isSafeToSpeculativelyExecute(), but we won't be able to do this with the current implementation.

Agreed, I just checked out my old patch, what I had done was:

I had added the nebulus bool PreservesOpCharacteristics which if true promised to not change the operands in a way the violated any of the analysis done in isSafeToSpeculativeExecute (although there was no real means of coordinating the analysis being done and the transform, I just eyeballed the ~50 instances I found in the code).

I also had

+/// Similiar to the above, but provide specific operands to be used in `I`. +/// NewOperands.size() must equal I.getNumOperands(). +bool isSafeToExecuteWithTheseOperands(const Instruction *I, + const ArrayRef<Use> NewOperands, + const Instruction *CtxI = nullptr, + AssumptionCache *AC = nullptr, + const DominatorTree *DT = nullptr, + const TargetLibraryInfo *TLI = nullptr);

but this was a bit naive, as constructing/destructing non-const operands is a pain.

Without creating unique helpers for the places that want to replace operands, I'm not really sure a way around either asking what operands or some nebulus contract that what they are doing doesn't violate the analysis.

Co-authored-by: Nikita Popov <[email protected]>

goldsteinn · 2024-07-18T15:11:16Z

I think we already discovered this issue as part of @goldsteinn's attempt to support non-constants in div speculation, but never followed up on it. Oops.

Yes, probably my fault.
IIRC I submitted a patch that essentially did this, but in the same patch I also touched 100 or so files enabling more aggressive analysis and it got lost in review xD

nikic

LGTM

llvm/include/llvm/Analysis/ValueTracking.h

llvm/lib/Analysis/ValueTracking.cpp

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp

Co-authored-by: Nikita Popov <[email protected]>

Summary: 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 <[email protected]> Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251182

dtcxzyw requested a review from nikic as a code owner July 18, 2024 13:38

llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Jul 18, 2024

dtcxzyw commented Jul 18, 2024

View reviewed changes

llvm/test/Transforms/InstCombine/select.ll Outdated Show resolved Hide resolved

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/select.ll Outdated Show resolved Hide resolved

nikic reviewed Jul 18, 2024

View reviewed changes

llvm/include/llvm/Analysis/ValueTracking.h Outdated Show resolved Hide resolved

llvm/test/Transforms/InstCombine/select.ll Outdated Show resolved Hide resolved

dtcxzyw added 2 commits July 18, 2024 22:25

[InstCombine] Add pre-commit tests. NFC.

3ecf076

[InstCombine] Do not use operand info in replaceInInstruction

4d557fd

dtcxzyw force-pushed the fix-pr99436 branch from ae2e781 to 4d557fd Compare July 18, 2024 14:33

nikic reviewed Jul 18, 2024

View reviewed changes

Update llvm/include/llvm/Analysis/ValueTracking.h

556c50a

Co-authored-by: Nikita Popov <[email protected]>

[InstCombine] Don't replace constant exprs

bc4eee1

nikic approved these changes Jul 21, 2024

View reviewed changes

llvm/include/llvm/Analysis/ValueTracking.h Outdated Show resolved Hide resolved

dtcxzyw commented Jul 21, 2024

View reviewed changes

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp Outdated Show resolved Hide resolved

[InstCombine] Address review comments. NFC.

e1e8722

Co-authored-by: Nikita Popov <[email protected]>

dtcxzyw merged commit 248fcab into llvm:main Jul 22, 2024
7 checks passed

dtcxzyw deleted the fix-pr99436 branch July 22, 2024 03:59

[InstCombine] Do not use operand info in replaceInInstruction #99492

[InstCombine] Do not use operand info in replaceInInstruction #99492

Uh oh!

Conversation

dtcxzyw commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

antoniofrighetto commented Jul 18, 2024

Uh oh!

dtcxzyw commented Jul 18, 2024

Uh oh!

Uh oh!

nikic Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

goldsteinn Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

goldsteinn Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

goldsteinn commented Jul 18, 2024

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[InstCombine] Do not use operand info in `replaceInInstruction` #99492

[InstCombine] Do not use operand info in `replaceInInstruction` #99492

dtcxzyw commented Jul 18, 2024 •

edited

Loading

llvmbot commented Jul 18, 2024 •

edited

Loading

goldsteinn Jul 18, 2024 •

edited

Loading