[llvm] [InstSimplify] Fix Inconsistent PHI Simplification (PR #113037)
Marius Kamp via llvm-commits
llvm-commits at lists.llvm.org
Sat Oct 19 06:10:27 PDT 2024
https://github.com/mskamp created https://github.com/llvm/llvm-project/pull/113037
Before this change, the simplification of PHI functions was inconsistent with
the simplification of `select` instructions. The simplification of `select`
instructions includes special handling of vector operands, which the
simplification of PHI functions lacked. This resulted in different
simplifications for partially `undef` vectors.
This change extends the simplification of PHI functions by handling also vector
operands that are partially `undef` or `poison`. As is done for `select`
operations, combining `undef` and `poison` elements now yields an `undef`
element.
This is consistent with the reasoning in Alive:
https://alive2.llvm.org/ce/z/vbmYTa
Adapt also a code generation test case. This adaption is necessary to keep the
loop structure even with the changed simplification of PHI functions, which
would otherwise simplify the control flow too much.
Fixes #98441
>From 337c9a40f46a1dbbdb8c59ee6aa42735a6b27ffa Mon Sep 17 00:00:00 2001
From: Marius Kamp <msk at posteo.org>
Date: Fri, 11 Oct 2024 10:47:57 +0200
Subject: [PATCH 1/3] [InstSimplify] Add Tests for PHI of Partially
Unknown/Poison Vector; NFC
---
.../InstSimplify/phi-vec-unknown-poison.ll | 118 ++++++++++++++++++
1 file changed, 118 insertions(+)
create mode 100644 llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll
diff --git a/llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll b/llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll
new file mode 100644
index 00000000000000..ca424af03e658d
--- /dev/null
+++ b/llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll
@@ -0,0 +1,118 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt < %s -passes=instsimplify -S | FileCheck %s
+
+define <2 x i1> @test_vec_undef_poison(i1 %c) {
+; CHECK-LABEL: define <2 x i1> @test_vec_undef_poison(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: br i1 [[C]], label %[[T:.*]], label %[[F:.*]]
+; CHECK: [[T]]:
+; CHECK-NEXT: br label %[[M:.*]]
+; CHECK: [[F]]:
+; CHECK-NEXT: br label %[[M]]
+; CHECK: [[M]]:
+; CHECK-NEXT: ret <2 x i1> <i1 poison, i1 false>
+;
+ br i1 %c, label %t, label %f
+t:
+ br label %m
+f:
+ br label %m
+m:
+ %res = phi <2 x i1> [ <i1 undef, i1 poison>, %t ], [ <i1 poison, i1 false>, %f ]
+ ret <2 x i1> %res
+}
+
+define <2 x i1> @test_vec_poison_undef(i1 %c) {
+; CHECK-LABEL: define <2 x i1> @test_vec_poison_undef(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: br i1 [[C]], label %[[T:.*]], label %[[F:.*]]
+; CHECK: [[T]]:
+; CHECK-NEXT: br label %[[M:.*]]
+; CHECK: [[F]]:
+; CHECK-NEXT: br label %[[M]]
+; CHECK: [[M]]:
+; CHECK-NEXT: ret <2 x i1> <i1 false, i1 poison>
+;
+ br i1 %c, label %t, label %f
+t:
+ br label %m
+f:
+ br label %m
+m:
+ %res = phi <2 x i1> [ <i1 false, i1 poison>, %t ], [ <i1 poison, i1 undef>, %f ]
+ ret <2 x i1> %res
+}
+
+define <2 x i1> @test_vec_poison_undef_defined(i1 %c1, i1 %c2) {
+; CHECK-LABEL: define <2 x i1> @test_vec_poison_undef_defined(
+; CHECK-SAME: i1 [[C1:%.*]], i1 [[C2:%.*]]) {
+; CHECK-NEXT: br i1 [[C1]], label %[[T:.*]], label %[[F:.*]]
+; CHECK: [[T]]:
+; CHECK-NEXT: br i1 [[C2]], label %[[TT:.*]], label %[[TF:.*]]
+; CHECK: [[TT]]:
+; CHECK-NEXT: br label %[[M:.*]]
+; CHECK: [[TF]]:
+; CHECK-NEXT: br label %[[M]]
+; CHECK: [[F]]:
+; CHECK-NEXT: br label %[[M]]
+; CHECK: [[M]]:
+; CHECK-NEXT: [[RES:%.*]] = phi <2 x i1> [ <i1 false, i1 poison>, %[[TT]] ], [ undef, %[[TF]] ], [ <i1 poison, i1 true>, %[[F]] ]
+; CHECK-NEXT: ret <2 x i1> [[RES]]
+;
+ br i1 %c1, label %t, label %f
+t:
+ br i1 %c2, label %tt, label %tf
+tt:
+ br label %m
+tf:
+ br label %m
+f:
+ br label %m
+m:
+ %res = phi <2 x i1> [ <i1 false, i1 poison>, %tt ], [ <i1 undef, i1 undef>, %tf ], [ <i1 poison, i1 true>, %f ]
+ ret <2 x i1> %res
+}
+
+define <2 x i8> @test_vec_same_constants(i1 %c) {
+; CHECK-LABEL: define <2 x i8> @test_vec_same_constants(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: br i1 [[C]], label %[[T:.*]], label %[[F:.*]]
+; CHECK: [[T]]:
+; CHECK-NEXT: br label %[[M:.*]]
+; CHECK: [[F]]:
+; CHECK-NEXT: br label %[[M]]
+; CHECK: [[M]]:
+; CHECK-NEXT: ret <2 x i8> <i8 4, i8 2>
+;
+ br i1 %c, label %t, label %f
+t:
+ br label %m
+f:
+ br label %m
+m:
+ %res = phi <2 x i8> [ <i8 4, i8 2>, %t ], [ <i8 4, i8 2>, %f ]
+ ret <2 x i8> %res
+}
+
+; Negative test
+define <2 x i8> @test_vec_different_constants(i1 %c) {
+; CHECK-LABEL: define <2 x i8> @test_vec_different_constants(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: br i1 [[C]], label %[[T:.*]], label %[[F:.*]]
+; CHECK: [[T]]:
+; CHECK-NEXT: br label %[[M:.*]]
+; CHECK: [[F]]:
+; CHECK-NEXT: br label %[[M]]
+; CHECK: [[M]]:
+; CHECK-NEXT: [[RES:%.*]] = phi <2 x i8> [ <i8 4, i8 2>, %[[T]] ], [ <i8 2, i8 4>, %[[F]] ]
+; CHECK-NEXT: ret <2 x i8> [[RES]]
+;
+ br i1 %c, label %t, label %f
+t:
+ br label %m
+f:
+ br label %m
+m:
+ %res = phi <2 x i8> [ <i8 4, i8 2>, %t ], [ <i8 2, i8 4>, %f ]
+ ret <2 x i8> %res
+}
>From 73cc6f71419fc0a10b8bed8ff41a6afaa9821b40 Mon Sep 17 00:00:00 2001
From: Marius Kamp <msk at posteo.org>
Date: Thu, 17 Oct 2024 19:59:48 +0200
Subject: [PATCH 2/3] [AMDGPU] Change Constant Vector in Test Case; NFC
This adaption is necessary to keep the loop structure even after
applying the next commit, which would otherwise simplify the control
flow too much.
---
llvm/test/CodeGen/AMDGPU/collapse-endcf.ll | 26 +++++++++++-----------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/llvm/test/CodeGen/AMDGPU/collapse-endcf.ll b/llvm/test/CodeGen/AMDGPU/collapse-endcf.ll
index 75d0b83a024ff5..5bf050f60fc010 100644
--- a/llvm/test/CodeGen/AMDGPU/collapse-endcf.ll
+++ b/llvm/test/CodeGen/AMDGPU/collapse-endcf.ll
@@ -983,7 +983,8 @@ define void @scc_liveness(i32 %arg) local_unnamed_addr #0 {
; GCN-NEXT: v_cmp_gt_i32_e32 vcc, s4, v0
; GCN-NEXT: s_mov_b32 s8, 0
; GCN-NEXT: v_cmp_eq_u32_e64 s[4:5], 0, v0
-; GCN-NEXT: s_mov_b64 s[12:13], 0
+; GCN-NEXT: s_mov_b64 s[14:15], 0
+; GCN-NEXT: s_mov_b32 s13, 1.0
; GCN-NEXT: s_mov_b64 s[6:7], 0
; GCN-NEXT: s_branch .LBB5_3
; GCN-NEXT: .LBB5_1: ; %Flow
@@ -991,11 +992,11 @@ define void @scc_liveness(i32 %arg) local_unnamed_addr #0 {
; GCN-NEXT: s_or_b64 exec, exec, s[10:11]
; GCN-NEXT: .LBB5_2: ; %bb10
; GCN-NEXT: ; in Loop: Header=BB5_3 Depth=1
-; GCN-NEXT: s_or_b64 exec, exec, s[14:15]
+; GCN-NEXT: s_or_b64 exec, exec, s[16:17]
; GCN-NEXT: s_and_b64 s[6:7], exec, s[4:5]
-; GCN-NEXT: s_or_b64 s[12:13], s[6:7], s[12:13]
+; GCN-NEXT: s_or_b64 s[14:15], s[6:7], s[14:15]
; GCN-NEXT: s_mov_b64 s[6:7], 0
-; GCN-NEXT: s_andn2_b64 exec, exec, s[12:13]
+; GCN-NEXT: s_andn2_b64 exec, exec, s[14:15]
; GCN-NEXT: s_cbranch_execz .LBB5_7
; GCN-NEXT: .LBB5_3: ; %bb1
; GCN-NEXT: ; =>This Inner Loop Header: Depth=1
@@ -1013,7 +1014,7 @@ define void @scc_liveness(i32 %arg) local_unnamed_addr #0 {
; GCN-NEXT: v_mov_b32_e32 v1, s9
; GCN-NEXT: v_mov_b32_e32 v2, s10
; GCN-NEXT: v_mov_b32_e32 v3, s11
-; GCN-NEXT: s_and_saveexec_b64 s[14:15], s[4:5]
+; GCN-NEXT: s_and_saveexec_b64 s[16:17], s[4:5]
; GCN-NEXT: s_cbranch_execz .LBB5_2
; GCN-NEXT: ; %bb.5: ; %bb4
; GCN-NEXT: ; in Loop: Header=BB5_3 Depth=1
@@ -1028,14 +1029,13 @@ define void @scc_liveness(i32 %arg) local_unnamed_addr #0 {
; GCN-NEXT: s_cbranch_execz .LBB5_1
; GCN-NEXT: ; %bb.6: ; %bb8
; GCN-NEXT: ; in Loop: Header=BB5_3 Depth=1
-; GCN-NEXT: s_mov_b32 s9, s8
-; GCN-NEXT: v_mov_b32_e32 v0, s8
-; GCN-NEXT: v_mov_b32_e32 v1, s9
-; GCN-NEXT: v_mov_b32_e32 v2, s10
-; GCN-NEXT: v_mov_b32_e32 v3, s11
+; GCN-NEXT: v_mov_b32_e32 v0, s12
+; GCN-NEXT: v_mov_b32_e32 v1, s13
+; GCN-NEXT: v_mov_b32_e32 v2, s14
+; GCN-NEXT: v_mov_b32_e32 v3, s15
; GCN-NEXT: s_branch .LBB5_1
; GCN-NEXT: .LBB5_7: ; %bb12
-; GCN-NEXT: s_or_b64 exec, exec, s[12:13]
+; GCN-NEXT: s_or_b64 exec, exec, s[14:15]
; GCN-NEXT: buffer_store_dword v3, v0, s[0:3], 0 offen
; GCN-NEXT: s_waitcnt vmcnt(0)
; GCN-NEXT: buffer_store_dword v2, v0, s[0:3], 0 offen
@@ -1168,7 +1168,7 @@ define void @scc_liveness(i32 %arg) local_unnamed_addr #0 {
; GCN-O0-NEXT: s_cbranch_execz .LBB5_6
; GCN-O0-NEXT: ; %bb.4: ; %bb8
; GCN-O0-NEXT: ; in Loop: Header=BB5_1 Depth=1
-; GCN-O0-NEXT: s_mov_b32 s10, 0
+; GCN-O0-NEXT: s_mov_b32 s10, 1.0
; GCN-O0-NEXT: ; implicit-def: $sgpr4
; GCN-O0-NEXT: ; implicit-def: $sgpr5
; GCN-O0-NEXT: ; implicit-def: $sgpr9
@@ -1372,7 +1372,7 @@ bb4: ; preds = %bb2
br i1 %tmp7, label %bb8, label %Flow
bb8: ; preds = %bb4
- %tmp9 = insertelement <4 x float> undef, float 0.0, i32 1
+ %tmp9 = insertelement <4 x float> undef, float 1.0, i32 1
br label %Flow
Flow: ; preds = %bb8, %bb4
>From d9477e43da6051f33551801ee0493bdd9f2219ba Mon Sep 17 00:00:00 2001
From: Marius Kamp <msk at posteo.org>
Date: Fri, 11 Oct 2024 11:25:17 +0200
Subject: [PATCH 3/3] [InstSimplify] Fix Inconsistent PHI Simplification
Before this commit, the simplification of PHI functions was inconsistent
with the simplification of `select` instructions. The simplification of
`select` instructions includes special handling of vector operands,
which the simplification of PHI functions lacked. This resulted in
different simplifications for partially `undef` vectors.
This commit extends the simplification of PHI functions by handling also
vector operands that are partially `undef` or `poison`. As is done for
`select` operations, combining `undef` and `poison` elements now yields
an `undef` element.
This is consistent with the reasoning in Alive:
https://alive2.llvm.org/ce/z/vbmYTa
Fixes #98441.
---
llvm/lib/Analysis/InstructionSimplify.cpp | 62 ++++++++++++++++---
.../InstSimplify/phi-vec-unknown-poison.ll | 7 +--
2 files changed, 57 insertions(+), 12 deletions(-)
diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp
index d08be1e55c853e..694138c93d6f12 100644
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
@@ -5288,6 +5288,54 @@ Value *llvm::simplifyExtractElementInst(Value *Vec, Value *Idx,
return ::simplifyExtractElementInst(Vec, Idx, Q, RecursionLimit);
}
+static Value *getCommonPHIValue(Value *PreviousCommon, Value *Current,
+ const SimplifyQuery &Q) {
+ if (!PreviousCommon)
+ return Current;
+
+ if (PreviousCommon == Current)
+ return PreviousCommon;
+
+ Constant *PreviousCommonC, *CurrentC;
+ if (match(PreviousCommon, m_Constant(PreviousCommonC)) &&
+ match(Current, m_Constant(CurrentC))) {
+ auto *VecTy = dyn_cast<FixedVectorType>(PreviousCommonC->getType());
+ if (VecTy) {
+ SmallVector<Constant *> NewCommonC;
+ unsigned NumElts = VecTy->getNumElements();
+ for (unsigned I = 0; I != NumElts; ++I) {
+ Constant *PrevElt = PreviousCommonC->getAggregateElement(I);
+ Constant *CurrElt = CurrentC->getAggregateElement(I);
+ if (!PrevElt || !CurrElt)
+ return nullptr;
+
+ if (PrevElt == CurrElt)
+ NewCommonC.push_back(PrevElt);
+ else if (isa<PoisonValue>(PrevElt) ||
+ (Q.isUndefValue(PrevElt) &&
+ isGuaranteedNotToBePoison(CurrElt)))
+ NewCommonC.push_back(CurrElt);
+ else if (isa<PoisonValue>(CurrElt) ||
+ (Q.isUndefValue(CurrElt) &&
+ isGuaranteedNotToBePoison(PrevElt)))
+ NewCommonC.push_back(PrevElt);
+ else
+ return nullptr;
+ }
+
+ return ConstantVector::get(NewCommonC);
+ }
+ }
+
+ if (Q.isUndefValue(PreviousCommon))
+ return Current;
+
+ if (Q.isUndefValue(Current))
+ return PreviousCommon;
+
+ return nullptr;
+}
+
/// See if we can fold the given phi. If not, returns null.
static Value *simplifyPHINode(PHINode *PN, ArrayRef<Value *> IncomingValues,
const SimplifyQuery &Q) {
@@ -5309,20 +5357,18 @@ static Value *simplifyPHINode(PHINode *PN, ArrayRef<Value *> IncomingValues,
continue;
}
if (Q.isUndefValue(Incoming)) {
- // Remember that we saw an undef value, but otherwise ignore them.
+ // Remember that we saw an undef value.
HasUndefInput = true;
- continue;
}
- if (CommonValue && Incoming != CommonValue)
+ CommonValue = getCommonPHIValue(CommonValue, Incoming, Q);
+ if (!CommonValue)
return nullptr; // Not the same, bail out.
- CommonValue = Incoming;
}
- // If CommonValue is null then all of the incoming values were either undef,
- // poison or equal to the phi node itself.
+ // If CommonValue is null then all of the incoming values were either poison
+ // or equal to the phi node itself.
if (!CommonValue)
- return HasUndefInput ? UndefValue::get(PN->getType())
- : PoisonValue::get(PN->getType());
+ return PoisonValue::get(PN->getType());
if (HasPoisonInput || HasUndefInput) {
// If we have a PHI node like phi(X, undef, X), where X is defined by some
diff --git a/llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll b/llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll
index ca424af03e658d..4b84417af187c7 100644
--- a/llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll
+++ b/llvm/test/Transforms/InstSimplify/phi-vec-unknown-poison.ll
@@ -10,7 +10,7 @@ define <2 x i1> @test_vec_undef_poison(i1 %c) {
; CHECK: [[F]]:
; CHECK-NEXT: br label %[[M]]
; CHECK: [[M]]:
-; CHECK-NEXT: ret <2 x i1> <i1 poison, i1 false>
+; CHECK-NEXT: ret <2 x i1> <i1 undef, i1 false>
;
br i1 %c, label %t, label %f
t:
@@ -31,7 +31,7 @@ define <2 x i1> @test_vec_poison_undef(i1 %c) {
; CHECK: [[F]]:
; CHECK-NEXT: br label %[[M]]
; CHECK: [[M]]:
-; CHECK-NEXT: ret <2 x i1> <i1 false, i1 poison>
+; CHECK-NEXT: ret <2 x i1> <i1 false, i1 undef>
;
br i1 %c, label %t, label %f
t:
@@ -56,8 +56,7 @@ define <2 x i1> @test_vec_poison_undef_defined(i1 %c1, i1 %c2) {
; CHECK: [[F]]:
; CHECK-NEXT: br label %[[M]]
; CHECK: [[M]]:
-; CHECK-NEXT: [[RES:%.*]] = phi <2 x i1> [ <i1 false, i1 poison>, %[[TT]] ], [ undef, %[[TF]] ], [ <i1 poison, i1 true>, %[[F]] ]
-; CHECK-NEXT: ret <2 x i1> [[RES]]
+; CHECK-NEXT: ret <2 x i1> <i1 false, i1 true>
;
br i1 %c1, label %t, label %f
t:
More information about the llvm-commits
mailing list