[NewGVN] Fix cyclic dependency in phi-of-ops #141510
Open
+141
−14
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-transforms Author: None (ManuelJBrito) ChangesThis is a continuation of #82110 this time for Phi-of-ops. For example in
Skipping the initial optimistic pass, we have the following defining expressions: Next iteration: Repeat Due to the phi-of-ops, add is no longer the class leader for add3. This causes the defining expression for phi to change, leading to not creating a phi-of-ops for add, making add the class leader for add3 once again. This cycle continues until the assertion is triggered." In essence, the problem is the cyclic dependency between phi, add, and add3: A simple way to prevent the cyclic dependency is to not use a phi-of-ops operand that comes after (RPO-wise) the instruction for which we are creating the phi-of-ops. This is a conservative fix, hence the regressions in completeness.ll. Full diff: https://github.com/llvm/llvm-project/pull/141510.diff 4 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 0a0ea65a1f036..706816c75826c 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -3847,7 +3847,10 @@ Value *NewGVN::findPHIOfOpsLeader(const Expression *E,
// Anything that isn't an instruction is always available.
if (!MemberInst)
return Member;
- if (DT->dominates(getBlockForValue(MemberInst), BB))
+ // Cannot use something that appears later in the iteration,
+ // as it may introduce a cyclic dependency.
+ if (DT->dominates(getBlockForValue(MemberInst), BB) &&
+ InstrToDFSNum(MemberInst) <= InstrToDFSNum(OrigInst))
return Member;
}
return nullptr;
diff --git a/llvm/test/Transforms/NewGVN/completeness.ll b/llvm/test/Transforms/NewGVN/completeness.ll
index 5585d2bf1e678..18c9be2290d24 100644
--- a/llvm/test/Transforms/NewGVN/completeness.ll
+++ b/llvm/test/Transforms/NewGVN/completeness.ll
@@ -227,17 +227,17 @@ define i64 @test5(i64 %arg) {
; CHECK: bb14:
; CHECK-NEXT: br label [[BB15:%.*]]
; CHECK: bb15:
-; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i64 [ [[TMP12]], [[BB14]] ], [ [[TMP25:%.*]], [[BB15]] ]
; CHECK-NEXT: [[TMP16:%.*]] = phi i64 [ [[TMP24:%.*]], [[BB15]] ], [ [[TMP11]], [[BB14]] ]
; CHECK-NEXT: [[TMP17:%.*]] = phi i64 [ [[TMP22:%.*]], [[BB15]] ], [ [[TMP10]], [[BB14]] ]
; CHECK-NEXT: [[TMP18:%.*]] = phi i64 [ [[TMP20:%.*]], [[BB15]] ], [ 0, [[BB14]] ]
-; CHECK-NEXT: store i64 [[PHIOFOPS]], ptr [[TMP]], align 8
+; CHECK-NEXT: [[TMP19:%.*]] = mul nsw i64 [[TMP16]], [[TMP17]]
+; CHECK-NEXT: store i64 [[TMP19]], ptr [[TMP]], align 8
; CHECK-NEXT: [[TMP20]] = add nuw nsw i64 [[TMP18]], 1
; CHECK-NEXT: [[TMP21:%.*]] = getelementptr inbounds [100 x i64], ptr @global, i64 0, i64 [[TMP20]]
; CHECK-NEXT: [[TMP22]] = load i64, ptr [[TMP21]], align 8
; CHECK-NEXT: [[TMP23:%.*]] = getelementptr inbounds [100 x i64], ptr @global.1, i64 0, i64 [[TMP20]]
; CHECK-NEXT: [[TMP24]] = load i64, ptr [[TMP23]], align 8
-; CHECK-NEXT: [[TMP25]] = mul nsw i64 [[TMP24]], [[TMP22]]
+; CHECK-NEXT: [[TMP25:%.*]] = mul nsw i64 [[TMP24]], [[TMP22]]
; CHECK-NEXT: [[TMP26:%.*]] = icmp eq i64 [[TMP20]], [[TMP25]]
; CHECK-NEXT: br i1 [[TMP26]], label [[BB4:%.*]], label [[BB15]]
; CHECK: bb27:
@@ -384,8 +384,10 @@ bb1: ; preds = %bb1, %bb
}
-;; Make sure we handle the case where we later come up with an expression that we need
-;; for a phi of ops.
+;; Handling the case where an expression from a previous iteration is
+;; needed for a phi of ops is tricky, as this expression might depend
+;; on the instruction for which we are creating the phi of ops,
+;; potentially creating a cyclic dependency. (See PR69211)
define void @test9(i1 %arg) {
; CHECK-LABEL: @test9(
; CHECK-NEXT: bb:
@@ -395,10 +397,10 @@ define void @test9(i1 %arg) {
; CHECK: bb2:
; CHECK-NEXT: br label [[BB6:%.*]]
; CHECK: bb6:
-; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i32 [ -13, [[BB2]] ], [ [[TMP11:%.*]], [[BB6]] ]
; CHECK-NEXT: [[TMP7:%.*]] = phi i32 [ 1, [[BB2]] ], [ [[TMP8:%.*]], [[BB6]] ]
; CHECK-NEXT: [[TMP8]] = add nuw nsw i32 [[TMP7]], 1
-; CHECK-NEXT: [[TMP11]] = add i32 -14, [[TMP8]]
+; CHECK-NEXT: [[TMP9:%.*]] = add i32 -14, [[TMP7]]
+; CHECK-NEXT: [[TMP11:%.*]] = add i32 -14, [[TMP8]]
; CHECK-NEXT: br label [[BB6]]
;
bb:
@@ -566,13 +568,13 @@ define void @test13() {
; CHECK-NEXT: [[TMP:%.*]] = load i8, ptr null, align 1
; CHECK-NEXT: br label [[BB3:%.*]]
; CHECK: bb3:
-; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i8 [ [[TMP]], [[BB1]] ], [ [[TMP10:%.*]], [[BB3]] ]
; CHECK-NEXT: [[TMP4:%.*]] = phi ptr [ null, [[BB1]] ], [ [[TMP6:%.*]], [[BB3]] ]
; CHECK-NEXT: [[TMP5:%.*]] = phi i32 [ undef, [[BB1]] ], [ [[TMP9:%.*]], [[BB3]] ]
; CHECK-NEXT: [[TMP6]] = getelementptr i8, ptr [[TMP4]], i64 1
-; CHECK-NEXT: [[TMP8:%.*]] = sext i8 [[PHIOFOPS]] to i32
+; CHECK-NEXT: [[TMP7:%.*]] = load i8, ptr [[TMP4]], align 1
+; CHECK-NEXT: [[TMP8:%.*]] = sext i8 [[TMP7]] to i32
; CHECK-NEXT: [[TMP9]] = mul i32 [[TMP5]], [[TMP8]]
-; CHECK-NEXT: [[TMP10]] = load i8, ptr [[TMP6]], align 1
+; CHECK-NEXT: [[TMP10:%.*]] = load i8, ptr [[TMP6]], align 1
; CHECK-NEXT: [[TMP11:%.*]] = icmp eq i8 [[TMP10]], 0
; CHECK-NEXT: br i1 [[TMP11]], label [[BB12:%.*]], label [[BB3]]
; CHECK: bb12:
diff --git a/llvm/test/Transforms/NewGVN/pr33461.ll b/llvm/test/Transforms/NewGVN/pr33461.ll
index 29d56c8245dd9..7ce67a067b1a6 100644
--- a/llvm/test/Transforms/NewGVN/pr33461.ll
+++ b/llvm/test/Transforms/NewGVN/pr33461.ll
@@ -8,12 +8,11 @@ define void @patatino() {
; CHECK-NEXT: entry:
; CHECK-NEXT: br i1 false, label [[FOR_COND1:%.*]], label [[FOR_INC:%.*]]
; CHECK: for.cond1:
-; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i16 [ poison, [[ENTRY:%.*]] ], [ [[INC:%.*]], [[FOR_INC]] ]
-; CHECK-NEXT: store i16 [[PHIOFOPS]], ptr @b, align 2
+; CHECK-NEXT: [[TMP1:%.*]] = load i16, ptr @b, align 2
; CHECK-NEXT: br label [[FOR_INC]]
; CHECK: for.inc:
; CHECK-NEXT: [[TMP0:%.*]] = load i16, ptr @b, align 2
-; CHECK-NEXT: [[INC]] = add i16 [[TMP0]], 1
+; CHECK-NEXT: [[INC:%.*]] = add i16 [[TMP0]], 1
; CHECK-NEXT: store i16 [[INC]], ptr @b, align 2
; CHECK-NEXT: br label [[FOR_COND1]]
;
diff --git a/llvm/test/Transforms/NewGVN/pr69211.ll b/llvm/test/Transforms/NewGVN/pr69211.ll
new file mode 100644
index 0000000000000..37a683c6238dc
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/pr69211.ll
@@ -0,0 +1,123 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=newgvn -S < %s | FileCheck %s
+
+define i64 @snork() {
+; CHECK-LABEL: define i64 @snork() {
+; CHECK-NEXT: [[BB:.*]]:
+; CHECK-NEXT: br label %[[BB1:.*]]
+; CHECK: [[BB1]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ 0, %[[BB]] ], [ [[ADD:%.*]], %[[BB1]] ]
+; CHECK-NEXT: [[ADD]] = add i64 [[PHI]], 1
+; CHECK-NEXT: [[ADD2:%.*]] = add i64 [[ADD]], 1
+; CHECK-NEXT: [[ICMP:%.*]] = icmp slt i64 [[ADD]], 100
+; CHECK-NEXT: br i1 [[ICMP]], label %[[BB1]], label %[[BB4:.*]]
+; CHECK: [[BB4]]:
+; CHECK-NEXT: ret i64 [[ADD2]]
+;
+bb:
+ br label %bb1
+
+bb1:
+ %phi = phi i64 [ 0, %bb ], [ %add3, %bb1 ]
+ %add = add i64 %phi, 1
+ %add2 = add i64 %add, 1
+ %add3 = add i64 %phi, 1
+ %icmp = icmp slt i64 %add3, 100
+ br i1 %icmp, label %bb1, label %bb4
+
+bb4:
+ ret i64 %add2
+}
+
+define i64 @wobble() {
+; CHECK-LABEL: define i64 @wobble() {
+; CHECK-NEXT: [[BB:.*]]:
+; CHECK-NEXT: br label %[[BB1:.*]]
+; CHECK: [[BB1]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ 0, %[[BB]] ], [ [[ADD:%.*]], %[[BB1]] ]
+; CHECK-NEXT: [[ADD]] = add i64 [[PHI]], 1
+; CHECK-NEXT: [[ADD2:%.*]] = add i64 [[ADD]], 1
+; CHECK-NEXT: [[ADD4:%.*]] = add i64 [[ADD2]], 1
+; CHECK-NEXT: [[ICMP:%.*]] = icmp slt i64 [[ADD]], 100
+; CHECK-NEXT: br i1 [[ICMP]], label %[[BB1]], label %[[BB6:.*]]
+; CHECK: [[BB6]]:
+; CHECK-NEXT: ret i64 [[ADD4]]
+;
+bb:
+ br label %bb1
+
+bb1:
+ %phi = phi i64 [ 0, %bb ], [ %add5, %bb1 ]
+ %add = add i64 %phi, 1
+ %add2 = add i64 %add, 1
+ %add3 = add i64 %add, 1
+ %add4 = add i64 %add2, 1
+ %add5 = add i64 %phi, 1
+ %icmp = icmp slt i64 %add5, 100
+ br i1 %icmp, label %bb1, label %bb6
+
+bb6:
+ ret i64 %add4
+}
+
+define i64 @wobble.1(i64 %arg) {
+; CHECK-LABEL: define i64 @wobble.1(
+; CHECK-SAME: i64 [[ARG:%.*]]) {
+; CHECK-NEXT: [[BB:.*]]:
+; CHECK-NEXT: br label %[[BB1:.*]]
+; CHECK: [[BB1]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ 0, %[[BB]] ], [ [[ADD:%.*]], %[[BB1]] ]
+; CHECK-NEXT: [[ADD]] = add i64 [[PHI]], 1
+; CHECK-NEXT: [[ADD2:%.*]] = add i64 [[ADD]], 1
+; CHECK-NEXT: [[ICMP:%.*]] = icmp slt i64 [[ADD]], 100
+; CHECK-NEXT: br i1 [[ICMP]], label %[[BB1]], label %[[BB5:.*]]
+; CHECK: [[BB5]]:
+; CHECK-NEXT: ret i64 [[ADD2]]
+;
+bb:
+ br label %bb1
+
+bb1:
+ %phi = phi i64 [ 0, %bb ], [ %add4, %bb1 ]
+ %add = add i64 %phi, 1
+ %add2 = add i64 %add, 1
+ %sub = sub i64 %add2, 1
+ %add3 = add i64 %sub, 1
+ %add4 = add i64 %phi, 1
+ %icmp = icmp slt i64 %add4, 100
+ br i1 %icmp, label %bb1, label %bb5
+
+bb5:
+ ret i64 %add3
+}
+
+define i64 @pluto(i64 %arg) {
+; CHECK-LABEL: define i64 @pluto(
+; CHECK-SAME: i64 [[ARG:%.*]]) {
+; CHECK-NEXT: [[BB:.*]]:
+; CHECK-NEXT: br label %[[BB1:.*]]
+; CHECK: [[BB1]]:
+; CHECK-NEXT: [[PHI:%.*]] = phi i64 [ 0, %[[BB]] ], [ [[ADD:%.*]], %[[BB1]] ]
+; CHECK-NEXT: [[ADD]] = add i64 [[PHI]], 1
+; CHECK-NEXT: [[ADD2:%.*]] = add i64 [[ADD]], 1
+; CHECK-NEXT: [[ICMP:%.*]] = icmp slt i64 [[ADD]], 100
+; CHECK-NEXT: br i1 [[ICMP]], label %[[BB1]], label %[[BB5:.*]]
+; CHECK: [[BB5]]:
+; CHECK-NEXT: ret i64 [[ADD2]]
+;
+bb:
+ br label %bb1
+
+bb1:
+ %phi = phi i64 [ 0, %bb ], [ %add4, %bb1 ]
+ %add = add i64 %phi, 1
+ %add2 = add i64 %add, 1
+ %sub = sub i64 %add2, 1
+ %add3 = add i64 %sub, 1
+ %add4 = add i64 %phi, 1
+ %icmp = icmp slt i64 %add4, 100
+ br i1 %icmp, label %bb1, label %bb5
+
+bb5:
+ ret i64 %add3
+}
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a continuation of #82110 this time for Phi-of-ops.
fixes #69211
For example in
@snorkinpr69211.llwe have the following:%phi = phi i64 [ 0, %bb ], [ %add3, %bb1 ]%add = add i64 %phi, 1%add2 = add i64 %add, 1%add3 = add i64 %phi, 1Skipping the initial optimistic pass, we have the following defining expressions:
phi -> [0, add3]
add -> 1 + phi
add2 -> 1 + add
add3 -> 1 + phi
Next iteration:
phi -> [0, add] (add is the class leader for add3)
add -> 1 + [0, add] -> [1, 1 + add] -> [1, add2]
add2 -> 1 + add (add3 becomes class leader)
add3 -> 1 + phi
Repeat
Due to the phi-of-ops, add is no longer the class leader for add3. This causes the defining expression for phi to change, leading to not creating a phi-of-ops for add, making add the class leader for add3 once again. This cycle continues until the assertion is triggered.
In essence, the problem is the cyclic dependency between phi, add, and add3:
phi -> add3, add3 -> add (leader), and add -> phi.
A simple way to prevent the cyclic dependency is to not use a phi-of-ops operand that comes after (RPO-wise) the instruction for which we are creating the phi-of-ops.
This is a conservative fix, hence the regressions in completeness.ll.
The ideal solution would be to perform SCC finding on the user-leader dependency graph, as these are the cases that lead to infinite evaluation loops.
From what I have seen, this is rare in practice, so probably not worth doing. This is something that can be revisited once NewGVN is more stable.