release/18.x: [InstSimplify] Make sure the simplified value doesn't generate poison in threadBinOpOverSelect (#87075)

[llvmbot]

Backport 3197f9d

Requested by: @dtcxzyw

[llvmbot]

@nikic What do you think about merging this PR to the release branch?

[llvmbot]

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: None (llvmbot)

Changes

Backport 3197f9d

Requested by: @dtcxzyw

---

Full diff: https://github.com/llvm/llvm-project/pull/88353.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 d0c27cae0dff99..72b6dfa181e86d 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -439,7 +439,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
+}

[nikic]

LGTM

[tstellar]

Hi @dtcxzyw (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix.

[dtcxzyw]

Hi @dtcxzyw (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix.

Fixed an incorrect poison-generating flag preservation in InstSimplify. It will fix a miscompilation on RISCV, which turns the incorrect or disjoint into an add.

Is that OK?