[llvm] 9ef8290 - [InstCombine] Fix buggy transform in `foldNestedSelects`; PR 71330

[Noah Goldstein via llvm-commits]

Author: Noah Goldstein
Date: 2023-11-09T16:36:49-06:00
New Revision: 9ef829097bbc4cf908698e3891af11a154e1d3e2

URL: https://github.com/llvm/llvm-project/commit/9ef829097bbc4cf908698e3891af11a154e1d3e2
DIFF: https://github.com/llvm/llvm-project/commit/9ef829097bbc4cf908698e3891af11a154e1d3e2.diff

LOG: [InstCombine] Fix buggy transform in `foldNestedSelects`; PR 71330

The bug is that `IsAndVariant` is used to assume which arm in the
select the output `SelInner` should be placed but match the inner
select condition with `m_c_LogicalOp`. With fully simplified ops, this
works fine, but its possible if the select condition is not
simplified, for it match both `LogicalAnd` and `LogicalOr` i.e `select
true, true, false`.

In PR71330 for example, the issue occurs in the following IR:
```
define i32 @bad() {
  %..i.i = select i1 false, i32 0, i32 3
  %brmerge = select i1 true, i1 true, i1 false
  %not.cmp.i.i.not = xor i1 true, true
  %.mux = zext i1 %not.cmp.i.i.not to i32
  %retval.0.i.i = select i1 %brmerge, i32 %.mux, i32 %..i.i
  ret i32 %retval.0.i.i
}
```

When simplifying:
```
%retval.0.i.i = select i1 %brmerge, i32 %.mux, i32 %..i.i
```

We end up matching `%brmerge` as `LogicalAnd` for `IsAndVariant`, but
the inner select (`%..i.i`) condition which is `false` with
`LogicalOr`.

Closes #71489

Added: 
    

Modified: 
    llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
    llvm/test/Transforms/InstCombine/pr71330.ll

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
index 9bd49f76d4bd5b7..71c2d68881441ac 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
@@ -2891,8 +2891,15 @@ static Instruction *foldNestedSelects(SelectInst &OuterSelVal,
     std::swap(InnerSel.TrueVal, InnerSel.FalseVal);
 
   Value *AltCond = nullptr;
-  auto matchOuterCond = [OuterSel, &AltCond](auto m_InnerCond) {
-    return match(OuterSel.Cond, m_c_LogicalOp(m_InnerCond, m_Value(AltCond)));
+  auto matchOuterCond = [OuterSel, IsAndVariant, &AltCond](auto m_InnerCond) {
+    // An unsimplified select condition can match both LogicalAnd and LogicalOr
+    // (select true, true, false). Since below we assume that LogicalAnd implies
+    // InnerSel match the FVal and vice versa for LogicalOr, we can't match the
+    // alternative pattern here.
+    return IsAndVariant ? match(OuterSel.Cond,
+                                m_c_LogicalAnd(m_InnerCond, m_Value(AltCond)))
+                        : match(OuterSel.Cond,
+                                m_c_LogicalOr(m_InnerCond, m_Value(AltCond)));
   };
 
   // Finally, match the condition that was driving the outermost `select`,

diff  --git a/llvm/test/Transforms/InstCombine/pr71330.ll b/llvm/test/Transforms/InstCombine/pr71330.ll
index 1620142523f757e..8eb98b2f6436b7e 100644
--- a/llvm/test/Transforms/InstCombine/pr71330.ll
+++ b/llvm/test/Transforms/InstCombine/pr71330.ll
@@ -15,8 +15,7 @@ define void @pr71330(i32 %conv, i1 %tobool19.not4, i16 %lb) {
 ; CHECK:       for.cond7.preheader.split.us.split:
 ; CHECK-NEXT:    ret void
 ; CHECK:       for.cond7:
-; CHECK-NEXT:    [[ADD9:%.*]] = add i32 [[CONV]], 3
-; CHECK-NEXT:    [[CMP12:%.*]] = icmp slt i32 [[ADD9]], 0
+; CHECK-NEXT:    [[CMP12:%.*]] = icmp slt i32 [[CONV]], 0
 ; CHECK-NEXT:    br i1 [[CMP12]], label [[FOR_BODY14:%.*]], label [[FOR_END25]]
 ; CHECK:       for.body14:
 ; CHECK-NEXT:    ret void