[InstCombine] Fix miscompilation in sinkNotIntoLogicalOp

[dtcxzyw]

Consider the following case:

define i1 @src(i8 %x) {
  %cmp = icmp slt i8 %x, -1
  %not1 = xor i1 %cmp, true
  %or = or i1 %cmp, %not1
  %not2 = xor i1 %or, true
  ret i1 %not2
}

sinkNotIntoLogicalOp(%or) calls freelyInvert(%cmp, /*IgnoredUser=*/%or) first. However, as %cmp is also used by Op1 = %not1, the RHS of %or is set to %cmp.not = xor i1 %cmp, true. Thus Op1 is out of date in the second call to freelyInvert. Similarly, the second call may change Op0. According to the analysis above, I decided to avoid this fold when one of the operands is also a user of the other.

Closes #142518.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Consider the following case:

define i1 @<!-- -->src(i8 %x) {
  %cmp = icmp slt i8 %x, -1
  %not1 = xor i1 %cmp, true
  %or = or i1 %cmp, %not1
  %not2 = xor i1 %or, true
  ret i1 %not2
}

sinkNotIntoLogicalOp(%or) calls freelyInvert(%cmp, /*IgnoredUser=*/%or) first. However, as %cmp is also used by Op1 = %not1, the RHS of %or is set to %cmp.not = xor i1 %cmp, true. Thus Op1 is out of date in the second call to freelyInvert. Similarly, the second call may change Op0. According to the analysis above, I decided to avoid this fold when one of the operands is also a user of the other.

Closes #142518.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp (+6)
• (added) llvm/test/Transforms/InstCombine/pr142518.ll (+43)

diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 2fb4bfecda8aa..c6c231f81c4ab 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -4513,6 +4513,12 @@ bool InstCombinerImpl::sinkNotIntoLogicalOp(Instruction &I) {
   if (Op0 == Op1)
     return false;
 
+  // If one of the operands is a user of the other,
+  // freelyInvert->freelyInvertAllUsersOf will change the operands of I, which
+  // may cause miscompilation.
+  if (match(Op0, m_Not(m_Specific(Op1))) || match(Op1, m_Not(m_Specific(Op0))))
+    return false;
+
   Instruction::BinaryOps NewOpc =
       match(&I, m_LogicalAnd()) ? Instruction::Or : Instruction::And;
   bool IsBinaryOp = isa<BinaryOperator>(I);
diff --git a/llvm/test/Transforms/InstCombine/pr142518.ll b/llvm/test/Transforms/InstCombine/pr142518.ll
new file mode 100644
index 0000000000000..980ed24b5eb98
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/pr142518.ll
@@ -0,0 +1,43 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+
+define i8 @pr142518(ptr %p, i8 %x, i1 %c) "instcombine-no-verify-fixpoint" {
+; CHECK-LABEL: define i8 @pr142518(
+; CHECK-SAME: ptr [[P:%.*]], i8 [[X:%.*]], i1 [[C:%.*]]) #[[ATTR0:[0-9]+]] {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i8 [[X]], -1
+; CHECK-NEXT:    br label %[[LOOP:.*]]
+; CHECK:       [[LOOP]]:
+; CHECK-NEXT:    [[NOT1:%.*]] = xor i1 [[CMP1]], true
+; CHECK-NEXT:    [[OR:%.*]] = or i1 [[CMP1]], [[NOT1]]
+; CHECK-NEXT:    [[CMP:%.*]] = xor i1 [[OR]], true
+; CHECK-NEXT:    [[EXT2:%.*]] = zext i1 [[CMP]] to i8
+; CHECK-NEXT:    store i8 [[EXT2]], ptr [[P]], align 1
+; CHECK-NEXT:    br i1 false, label %[[LOOP]], label %[[EXIT:.*]]
+; CHECK:       [[EXIT]]:
+; CHECK-NEXT:    [[NOT3:%.*]] = xor i1 [[OR]], true
+; CHECK-NEXT:    [[EXT3:%.*]] = zext i1 [[NOT3]] to i8
+; CHECK-NEXT:    ret i8 [[EXT3]]
+;
+entry:
+  %flag = alloca i8, align 1
+  %cmp = icmp slt i8 %x, -1
+  br label %loop
+
+loop:
+  %phi = phi i1 [ %cmp, %entry ], [ %c, %loop ]
+  %not1 = xor i1 %phi, true
+  %or = or i1 %cmp, %not1
+  %not2 = xor i1 %or, true
+  %ext2 = zext i1 %not2 to i8
+  store i8 %ext2, ptr %p, align 1
+  store i8 1, ptr %flag, align 1
+  %flagv = load i8, ptr %flag, align 1
+  %cond = icmp eq i8 %flagv, 0
+  br i1 %cond, label %loop, label %exit
+
+exit:
+  %not3 = xor i1 %or, true
+  %ext3 = zext i1 %not3 to i8
+  ret i8 %ext3
+}

[nikic]

Can this be fixed by replacing IgnoredUser with IgnoredUse?

[nikic]

Can this be fixed by replacing IgnoredUser with IgnoredUse?

Hm, I guess not. But I'm also not sure the proposed fix is complete, as there may be other patterns with inter-dependencies.

Generally, what this transform does is really terrible :(

[nikic]

LGTM

This is not great, but I don't have better ideas.