[llvm] [InstCombine] Fix buggy transform in foldNestedSelects; PR 71330 (PR #71489)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 9 02:54:20 PST 2023
https://github.com/goldsteinn updated https://github.com/llvm/llvm-project/pull/71489
>From 764cdf80d00558d7944344a321f1ca1deed9b986 Mon Sep 17 00:00:00 2001
From: Noah Goldstein <goldstein.w.n at gmail.com>
Date: Mon, 6 Nov 2023 23:24:50 -0600
Subject: [PATCH 1/2] [InstCombine] Add reduced test for PR 71330; NFC
---
llvm/test/Transforms/InstCombine/pr71330.ll | 77 +++++++++++++++++++++
1 file changed, 77 insertions(+)
create mode 100644 llvm/test/Transforms/InstCombine/pr71330.ll
diff --git a/llvm/test/Transforms/InstCombine/pr71330.ll b/llvm/test/Transforms/InstCombine/pr71330.ll
new file mode 100644
index 000000000000000..1620142523f757e
--- /dev/null
+++ b/llvm/test/Transforms/InstCombine/pr71330.ll
@@ -0,0 +1,77 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 3
+; RUN: opt -passes=instcombine -S < %s | FileCheck %s
+
+define void @pr71330(i32 %conv, i1 %tobool19.not4, i16 %lb) {
+; CHECK-LABEL: define void @pr71330(
+; CHECK-SAME: i32 [[CONV:%.*]], i1 [[TOBOOL19_NOT4:%.*]], i16 [[LB:%.*]]) {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label [[FOR_COND7_PREHEADER:%.*]]
+; CHECK: for.cond7.preheader:
+; CHECK-NEXT: br i1 [[TOBOOL19_NOT4]], label [[FOR_COND7_PREHEADER_SPLIT_US:%.*]], label [[FOR_COND7:%.*]]
+; CHECK: for.cond7.preheader.split.us:
+; CHECK-NEXT: br i1 true, label [[FOR_COND7_PREHEADER_SPLIT_US_SPLIT:%.*]], label [[FOR_COND7_US_US:%.*]]
+; CHECK: for.cond7.us.us:
+; CHECK-NEXT: br i1 poison, label [[FOR_COND7_US_US]], label [[FOR_END25:%.*]]
+; 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: br i1 [[CMP12]], label [[FOR_BODY14:%.*]], label [[FOR_END25]]
+; CHECK: for.body14:
+; CHECK-NEXT: ret void
+; CHECK: for.end25:
+; CHECK-NEXT: br i1 false, label [[FOR_COND7_PREHEADER]], label [[FOR_END36:%.*]]
+; CHECK: for.end36:
+; CHECK-NEXT: ret void
+;
+entry:
+ br label %for.cond7.preheader
+
+for.cond7.preheader: ; preds = %for.end25, %entry
+ %storemerge33 = phi i32 [ -3, %entry ], [ 0, %for.end25 ]
+ %sm8 = and i32 %storemerge33, 1
+ %add = add i32 %storemerge33, 8
+ %cmp.i.i.not = icmp eq i32 %sm8, 0
+ %cmp3.i.i = icmp eq i32 %add, 0
+ %and7.i.i = and i32 %storemerge33, 1
+ %cmp8.i.i = icmp eq i32 %and7.i.i, 0
+ %tobool.not.i.i = icmp eq i32 %add, 0
+ %..i.i = select i1 %tobool.not.i.i, i32 1, i32 3
+ br i1 %tobool19.not4, label %for.cond7.preheader.split.us, label %for.cond7
+
+for.cond7.preheader.split.us: ; preds = %for.cond7.preheader
+ br i1 %cmp.i.i.not, label %for.cond7.us.us, label %for.cond7.preheader.split.us.split
+
+for.cond7.us.us: ; preds = %for.cond7.us.us, %for.cond7.preheader.split.us
+ %spec.select = select i1 %cmp8.i.i, i32 1, i32 %..i.i
+ %retval.0.i.i.us.us = select i1 %cmp3.i.i, i32 0, i32 %spec.select
+ %add9.us.us = add i32 %retval.0.i.i.us.us, %conv
+ %conv10.us.us = sext i32 %add9.us.us to i64
+ %cmp12.us.us = icmp slt i64 %conv10.us.us, 0
+ br i1 %cmp12.us.us, label %for.cond7.us.us, label %for.end25
+
+for.cond7.preheader.split.us.split: ; preds = %for.cond7.preheader.split.us
+ ret void
+
+for.cond7: ; preds = %for.cond7.preheader
+ %cmp.i.i.not.not = xor i1 %cmp.i.i.not, true
+ %brmerge = select i1 %cmp.i.i.not.not, i1 true, i1 %cmp3.i.i
+ %spec.select34 = select i1 %cmp8.i.i, i32 1, i32 %..i.i
+ %retval.0.i.i = select i1 %brmerge, i32 0, i32 %spec.select34
+ %add9 = add i32 %retval.0.i.i, %conv
+ %conv10 = sext i32 %add9 to i64
+ %cmp12 = icmp slt i64 %conv10, 0
+ br i1 %cmp12, label %for.body14, label %for.end25
+
+for.body14: ; preds = %for.cond7
+ ret void
+
+for.end25: ; preds = %for.cond7, %for.cond7.us.us
+ %conv35 = zext i16 %lb to i32
+ %cmp = icmp slt i32 %conv35, 0
+ br i1 %cmp, label %for.cond7.preheader, label %for.end36
+
+for.end36: ; preds = %for.end25
+ ret void
+}
>From f169726951e2e3534b7629d72abdc1ec07e9262c Mon Sep 17 00:00:00 2001
From: Noah Goldstein <goldstein.w.n at gmail.com>
Date: Tue, 7 Nov 2023 13:40:01 -0600
Subject: [PATCH 2/2] [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`.
---
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp | 11 +++++++++--
llvm/test/Transforms/InstCombine/pr71330.ll | 3 +--
2 files changed, 10 insertions(+), 4 deletions(-)
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
More information about the llvm-commits
mailing list