[InstSimplify] Make sure the simplified value doesn't generate poison in threadBinOpOverSelect

[dtcxzyw]

Alive2: https://alive2.llvm.org/ce/z/y_Jmdn
Fix #87042.

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Alive2: https://alive2.llvm.org/ce/z/y_Jmdn
Fix #87042.

---

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

2 Files Affected:

• (modified) llvm/lib/Analysis/InstructionSimplify.cpp (+2-1)
• (added) llvm/test/Transforms/InstSimplify/pr87042.ll (+42)

diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index 9ff3faff799027..3c943a09a9c232 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -440,7 +440,8 @@ static Value *threadBinOpOverSelect(Instruction::BinaryOps Opcode, Value *LHS,
     // Check that the simplified value has the form "X op Y" where "op" is the
     // same as the original operation.
     Instruction *Simplified = dyn_cast<Instruction>(FV ? FV : TV);
-    if (Simplified && Simplified->getOpcode() == unsigned(Opcode)) {
+    if (Simplified && Simplified->getOpcode() == unsigned(Opcode) &&
+        !Simplified->hasPoisonGeneratingFlags()) {
       // The value that didn't simplify is "UnsimplifiedLHS op UnsimplifiedRHS".
       // We already know that "op" is the same as for the simplified value.  See
       // if the operands match too.  If so, return the simplified value.
diff --git a/llvm/test/Transforms/InstSimplify/pr87042.ll b/llvm/test/Transforms/InstSimplify/pr87042.ll
new file mode 100644
index 00000000000000..800d27c9e65043
--- /dev/null
+++ b/llvm/test/Transforms/InstSimplify/pr87042.ll
@@ -0,0 +1,42 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -passes=instsimplify -S | FileCheck %s
+
+; %or2 cannot be folded into %or1 because %or1 has disjoint.
+; TODO: Can we move the logic into InstCombine and drop the disjoint flag?
+define i64 @test(i1 %cond, i64 %x) {
+; CHECK-LABEL: define i64 @test(
+; CHECK-SAME: i1 [[COND:%.*]], i64 [[X:%.*]]) {
+; CHECK-NEXT:    [[OR1:%.*]] = or disjoint i64 [[X]], 7
+; CHECK-NEXT:    [[SEL1:%.*]] = select i1 [[COND]], i64 [[OR1]], i64 [[X]]
+; CHECK-NEXT:    [[OR2:%.*]] = or i64 [[SEL1]], 7
+; CHECK-NEXT:    ret i64 [[OR2]]
+;
+  %or1 = or disjoint i64 %x, 7
+  %sel1 = select i1 %cond, i64 %or1, i64 %x
+  %or2 = or i64 %sel1, 7
+  ret i64 %or2
+}
+
+define i64 @pr87042(i64 %x) {
+; CHECK-LABEL: define i64 @pr87042(
+; CHECK-SAME: i64 [[X:%.*]]) {
+; CHECK-NEXT:    [[AND1:%.*]] = and i64 [[X]], 65535
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp eq i64 [[AND1]], 0
+; CHECK-NEXT:    [[OR1:%.*]] = or disjoint i64 [[X]], 7
+; CHECK-NEXT:    [[SEL1:%.*]] = select i1 [[CMP1]], i64 [[OR1]], i64 [[X]]
+; CHECK-NEXT:    [[AND2:%.*]] = and i64 [[SEL1]], 16776960
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp eq i64 [[AND2]], 0
+; CHECK-NEXT:    [[OR2:%.*]] = or i64 [[SEL1]], 7
+; CHECK-NEXT:    [[SEL2:%.*]] = select i1 [[CMP2]], i64 [[OR2]], i64 [[SEL1]]
+; CHECK-NEXT:    ret i64 [[SEL2]]
+;
+  %and1 = and i64 %x, 65535
+  %cmp1 = icmp eq i64 %and1, 0
+  %or1 = or disjoint i64 %x, 7
+  %sel1 = select i1 %cmp1, i64 %or1, i64 %x
+  %and2 = and i64 %sel1, 16776960
+  %cmp2 = icmp eq i64 %and2, 0
+  %or2 = or i64 %sel1, 7
+  %sel2 = select i1 %cmp2, i64 %or2, i64 %sel1
+  ret i64 %sel2
+}

[goldsteinn]

Shouldn't isSafeToSpeculativelyExecute suffice here?

[dtcxzyw]

isSafeToSpeculativelyExecute only cares about immediate UB instead of poison.

[goldsteinn]

why is the fix to prohibit any poison flag and not to just drop the poison generating flag?

[dtcxzyw]

IIRC modifying an instruction is not allowed in InstSimplify :(

[goldsteinn]

oh yeah xD

Maybe just move this to InstCombine? Or put equivelent code that drops poison generating flags there?

[goldsteinn]

LGTM

[nikic]

Can you please check what the impact of just deleting this special case entirely is? IIRC the last time I looked at this highly dubious code it didn't have any test coverage at all (though I might have added some at the time).

[dtcxzyw]

Emm, it would be better to move the logic into InstCombine.

[goldsteinn]

Is this dead?

[dtcxzyw]

Is this dead?

No. TBH I prefer this patch because I think the regression is acceptable.

[nikic]

LGTM

[dtcxzyw]

/cherry-pick 3197f9d

[llvmbot]

/pull-request #88353