[InstCombine] Fix infinite loop due to incorrect DoesConsume

[dtcxzyw]

When a call to getFreelyInvertedImpl with a select/phi node fails, DoesConsume should not be changed.

Fixes #82877.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Yingwei Zheng (dtcxzyw)

Changes

Without the invalidation, the transform in both directions are allowed, which causes InstCombine to hang.
Fixes #82877.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/InstCombine/InstructionCombining.cpp (+4)
• (added) llvm/test/Transforms/InstCombine/pr82877.ll (+40)

diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index 87c8dca7efed89..96afd0380ac22b 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2295,9 +2295,11 @@ Value *InstCombiner::getFreelyInvertedImpl(Value *V, bool WillInvertAllUses,
   // If `V` is of the form `A + B` then `-1 - V` can be folded into
   // `(-1 - B) - A` if we are willing to invert all of the uses.
   if (match(V, m_Add(m_Value(A), m_Value(B)))) {
+    bool DoesConsumeOldValue = DoesConsume;
     if (auto *BV = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
                                          DoesConsume, Depth))
       return Builder ? Builder->CreateSub(BV, A) : NonNull;
+    DoesConsume = DoesConsumeOldValue;
     if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
                                          DoesConsume, Depth))
       return Builder ? Builder->CreateSub(AV, B) : NonNull;
@@ -2307,9 +2309,11 @@ Value *InstCombiner::getFreelyInvertedImpl(Value *V, bool WillInvertAllUses,
   // If `V` is of the form `A ^ ~B` then `~(A ^ ~B)` can be folded
   // into `A ^ B` if we are willing to invert all of the uses.
   if (match(V, m_Xor(m_Value(A), m_Value(B)))) {
+    bool DoesConsumeOldValue = DoesConsume;
     if (auto *BV = getFreelyInvertedImpl(B, B->hasOneUse(), Builder,
                                          DoesConsume, Depth))
       return Builder ? Builder->CreateXor(A, BV) : NonNull;
+    DoesConsume = DoesConsumeOldValue;
     if (auto *AV = getFreelyInvertedImpl(A, A->hasOneUse(), Builder,
                                          DoesConsume, Depth))
       return Builder ? Builder->CreateXor(AV, B) : NonNull;
diff --git a/llvm/test/Transforms/InstCombine/pr82877.ll b/llvm/test/Transforms/InstCombine/pr82877.ll
new file mode 100644
index 00000000000000..64ef490cb588d8
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/pr82877.ll
@@ -0,0 +1,40 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -S -passes=instcombine < %s | FileCheck %s
+
+define i64 @func(i32 %p) {
+; CHECK-LABEL: define i64 @func(
+; CHECK-SAME: i32 [[P:%.*]]) {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[NOT:%.*]] = xor i32 [[P]], -1
+; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    [[P0:%.*]] = phi i32 [ [[NOT]], [[ENTRY:%.*]] ], [ [[CONV:%.*]], [[FOR_BODY]] ]
+; CHECK-NEXT:    [[P1:%.*]] = phi i32 [ 0, [[ENTRY]] ], [ [[INC:%.*]], [[FOR_BODY]] ]
+; CHECK-NEXT:    [[INC]] = add i32 [[P1]], 1
+; CHECK-NEXT:    [[CMP1_NOT:%.*]] = icmp eq i32 [[INC]], 0
+; CHECK-NEXT:    [[COND:%.*]] = select i1 [[CMP1_NOT]], i32 -1231558963, i32 0
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[COND]], [[P0]]
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp ne i32 [[XOR]], -2
+; CHECK-NEXT:    [[CONV]] = zext i1 [[CMP2]] to i32
+; CHECK-NEXT:    br i1 [[CMP2]], label [[FOR_BODY]], label [[FOR_EXIT:%.*]]
+; CHECK:       for.exit:
+; CHECK-NEXT:    ret i64 0
+;
+entry:
+  %not = xor i32 %p, -1
+  br label %for.body
+
+for.body:
+  %p0 = phi i32 [ %not, %entry ], [ %conv, %for.body ]
+  %p1 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
+  %inc = add i32 %p1, 1
+  %cmp1 = icmp ne i32 %inc, 0
+  %cond = select i1 %cmp1, i32 0, i32 -1231558963
+  %xor = xor i32 %cond, %p0
+  %cmp2 = icmp ne i32 %xor, -2
+  %conv = zext i1 %cmp2 to i32
+  br i1 %cmp2, label %for.body, label %for.exit
+
+for.exit:
+  ret i64 0
+}

[goldsteinn]

Still trying to properly understand why this is the fix, but something to note:
below at line:2377 seems buggy.

   if (NewIncomingVal == V)
        return nullptr;

NewIncomingVal uses builder as nullptr so if the return is non-null, it will return NonNull which will never equal V

[dtcxzyw]

See also the comment #80804 (comment).

[goldsteinn]

think the Builder == nullptr case should return V (which is equally valid as NonNull).
I see that it ends up working b.c the force of recursive depth to the end and /*WillInvertAllUses=*/false will make it only match not/const. But should be fixed up I think.

Not related to the current bug though.

[goldsteinn]

At the very least should add an assert that NewIncomingVal != NonNull

[dtcxzyw]

Still trying to properly understand why this is the fix, but something to note: below at line:2377 seems buggy.

   if (NewIncomingVal == V)
        return nullptr;

NewIncomingVal uses builder as nullptr so if the return is non-null, it will return NonNull which will never equal V

I believe it never happens since NewIncomingVal always evaluates to nullptr if U.get() is also a phi node.

[goldsteinn]

I don't think the code is explicitly buggy now, but it seems like an accident waiting to happen. Although it won't be silent (a v explicit segfault), so guess there is no real alarm.

[goldsteinn]

LGMT.

[dtcxzyw]

Ping. @nikic

[dtcxzyw]

Ping. @nikic

[nikic]

LGTM