[NewGVN] Prevent cyclic reference when building phiofops

[XChy]

Fixes #69211.
This patch adds cyclic reference check in findPHIOfOpsLeader.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: XChy (XChy)

Changes

Fixes #69211.
This patch adds self-reference check in findPHIOfOpsLeader.

---

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

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+10)
• (modified) llvm/test/Transforms/NewGVN/cyclic-phi-handling.ll (+68-6)

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 19ac9526b5f88b6..2bd5e2127aa340a 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -3812,6 +3812,16 @@ Value *NewGVN::findPHIOfOpsLeader(const Expression *E,
     auto *MemberInst = dyn_cast<Instruction>(Member);
     if (MemberInst == OrigInst)
       continue;
+
+    // Prevent cyclic reference, such as:
+    // %a = add i64 %phi, 1
+    // %foundinst = add i64 %a, 1
+    // =>
+    // %phiofops = phi i64 [1, %BB1], [%foundinst, %BB2]
+    // %foundinst = add i64 %phiofops, 1
+    if (llvm::find(MemberInst->operands(), OrigInst) != MemberInst->op_end())
+      continue;
+
     // Anything that isn't an instruction is always available.
     if (!MemberInst)
       return Member;
diff --git a/llvm/test/Transforms/NewGVN/cyclic-phi-handling.ll b/llvm/test/Transforms/NewGVN/cyclic-phi-handling.ll
index 4a2f0b972c9fe1a..5f8d90aa4a6aa21 100644
--- a/llvm/test/Transforms/NewGVN/cyclic-phi-handling.ll
+++ b/llvm/test/Transforms/NewGVN/cyclic-phi-handling.ll
@@ -5,15 +5,15 @@ target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 define void @foo(i32 %arg, i32 %arg1, ptr %arg2) {
 ; CHECK-LABEL: @foo(
 ; CHECK-NEXT:  bb:
-; CHECK-NEXT:    br label %bb3
+; CHECK-NEXT:    br label [[BB3:%.*]]
 ; CHECK:       bb3:
-; CHECK-NEXT:    [[TMP:%.*]] = phi i32 [ %arg1, %bb ], [ [[TMP:%.*]]4, %bb7 ]
-; CHECK-NEXT:    [[TMP4:%.*]] = phi i32 [ %arg, %bb ], [ [[TMP]], %bb7 ]
-; CHECK-NEXT:    [[TMP5:%.*]] = call i32 %arg2(i32 [[TMP4]], i32 [[TMP]])
+; CHECK-NEXT:    [[TMP:%.*]] = phi i32 [ [[ARG1:%.*]], [[BB:%.*]] ], [ [[TMP4:%.*]], [[BB7:%.*]] ]
+; CHECK-NEXT:    [[TMP4]] = phi i32 [ [[ARG:%.*]], [[BB]] ], [ [[TMP]], [[BB7]] ]
+; CHECK-NEXT:    [[TMP5:%.*]] = call i32 [[ARG2:%.*]](i32 [[TMP4]], i32 [[TMP]])
 ; CHECK-NEXT:    [[TMP6:%.*]] = icmp ne i32 [[TMP5]], 0
-; CHECK-NEXT:    br i1 [[TMP6]], label %bb7, label %bb8
+; CHECK-NEXT:    br i1 [[TMP6]], label [[BB7]], label [[BB8:%.*]]
 ; CHECK:       bb7:
-; CHECK-NEXT:    br label %bb3
+; CHECK-NEXT:    br label [[BB3]]
 ; CHECK:       bb8:
 ; CHECK-NEXT:    ret void
 ;
@@ -35,3 +35,65 @@ bb7:                                              ; preds = %bb3
 bb8:                                              ; preds = %bb3
   ret void
 }
+
+; Don't let %b be the candidate when making %a phi of ops
+define i64 @phis_of_ops_cyclic() {
+; CHECK-LABEL: @phis_of_ops_cyclic(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[A:%.*]], [[FOR_BODY]] ]
+; CHECK-NEXT:    [[A]] = add i64 [[INDVARS_IV]], 1
+; CHECK-NEXT:    [[B:%.*]] = add i64 [[A]], 1
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp slt i64 [[A]], 100
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[FOR_BODY]], label [[END:%.*]]
+; CHECK:       end:
+; CHECK-NEXT:    ret i64 [[B]]
+;
+entry:
+  br label %for.body
+
+for.body:                                        ; preds = %for.body, %entry
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %a = add i64 %indvars.iv, 1
+  %b = add i64 %a, 1
+
+  %indvars.iv.next = add i64 %indvars.iv, 1
+  %tobool = icmp slt i64 %indvars.iv.next, 100
+  br i1 %tobool, label %for.body, label %end
+
+end:
+  ret i64 %b
+}
+
+define i64 @phis_of_ops_cyclic_multiple() {
+; CHECK-LABEL: @phis_of_ops_cyclic_multiple(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_BODY:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    [[INDVARS_IV:%.*]] = phi i64 [ 0, [[ENTRY:%.*]] ], [ [[A:%.*]], [[FOR_BODY]] ]
+; CHECK-NEXT:    [[A]] = add i64 [[INDVARS_IV]], 1
+; CHECK-NEXT:    [[B:%.*]] = add i64 [[A]], 1
+; CHECK-NEXT:    [[D:%.*]] = add i64 [[B]], 1
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp slt i64 [[A]], 100
+; CHECK-NEXT:    br i1 [[TOBOOL]], label [[FOR_BODY]], label [[END:%.*]]
+; CHECK:       end:
+; CHECK-NEXT:    ret i64 [[D]]
+;
+entry:
+  br label %for.body
+
+for.body:                                        ; preds = %for.body, %entry
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %a = add i64 %indvars.iv, 1
+  %b = add i64 %a, 1
+  %c = add i64 %a, 1
+  %d = add i64 %b, 1
+
+  %indvars.iv.next = add i64 %indvars.iv, 1
+  %tobool = icmp slt i64 %indvars.iv.next, 100
+  br i1 %tobool, label %for.body, label %end
+
+end:
+  ret i64 %d
+}

[nikic]

Not familiar enough with this code to review.

[nikic]

is_contained

[ManuelJBrito]

Thanks for working on this. This is the same issue as in #60439.
I am not sure this patch covers all cases, here we only check if the leader is an operand of the original instruction, direct dependence, but the leader might, for example, be an operand of one of the operands of the original instruction.
Or an even more contrived case would be if one of the operands of the leader was the leader of one of the operands of the original instruction.
I don't have any concrete examples of this happening and causing NewGVN to not terminate, but it should be possible.
I think this requires performing SCC finding on a dependence graph with both user and leader dependencies.

[XChy]

For example, be an operand of one of the operands of the original instruction. Or an even more contrived case would be if one of the operands of the leader was the leader of one of the operands of the original instruction. I don't have any concrete examples of this happening and causing NewGVN to not terminate

For the first case, I think that's correct. I missed here. There is a case:

define i64 @foo(i64 %e) {
entry:
  br label %for.body

for.body:                                        ; preds = %for.body, %entry
  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
  %original = add i64 %indvars.iv, 1

  %b = add i64 %original, 1
  %c = sub i64 %b, 1

  %phileader = add i64 %c, 1

  %indvars.iv.next = add i64 %indvars.iv, 1
  %tobool = icmp slt i64 %indvars.iv.next, 100
  br i1 %tobool, label %for.body, label %end

end:
  ret i64 %phileader
}

But for the latter, I think that's OK. Based on my knowledge about this algorithm, if operand of phiopsleader is equivalent/congruent to the operand of orig. For example(May not so precise):

declare void @use(i64)
define i64 @foo(i64 %operand_lead) {
entry:
  br label %for.body

for.body:                                        ; preds = %for.body, %entry
  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]

  %operand_mid = add i64 %operand_lead, 1
  %operand = sub i64 %operand_lead, 1
  %original = add i64 %indvars.iv, %operand
  %phi_leader = add i64 %indvars.iv, %operand_lead

  call void @use(i64 %original)
  call void @use(i64 %phi_leader)

  %indvars.iv.next = add i64 %indvars.iv, %operand

  %tobool = icmp slt i64 %indvars.iv.next, 100
  br i1 %tobool, label %for.body, label %end

end:
  ret i64 0
}

It would be fine for me to merge phiopsleader into new orig.

[XChy]

I tried performing SCC finding starting from the leader of OrigInst.

  TarjanSCC CycleFinder;
  DenseMap<const Value *, SmallVector<Value *, 4>> Edges;
  Value *OrigInstLeader =
      lookupOperandLeader(const_cast<Value *>((const Value *)OrigInst));
  auto *LeaderInst = dyn_cast<Instruction>(OrigInstLeader);

  SmallVector<Value *, 4> Members(CC->begin(), CC->end());
  Edges[OrigInst] = Members;

  CycleFinder.Start(OrigInst, Edges);
  if (LeaderInst) {
    Edges[LeaderInst] = Members;
    CycleFinder.Start(LeaderInst, Edges);
  }

But it would lead to the fact that we could possibly optimize the IR after eliminate all the redundancies. Example:

define i64 @test5() {
bb:
  %tmp10 = load i64, ptr null, align 16
  %tmp12 = mul i64 %tmp10, %tmp10
  %tmp13 = icmp eq i64 %tmp12, 0
  br label %bb15

bb15:                                             ; preds = %bb15, %bb
  %tmp16 = phi i64 [ 0, %bb15 ], [ %tmp10, %bb ]
  %tmp19 = mul i64 %tmp16, %tmp10
  store i64 %tmp19, ptr null, align 8
  br label %bb15
}

[XChy]

Resolved comment.
When Member == OrigInstLeader, it's legal to make it a candidate.