[MachineSink] Check predecessor/successor relationship between two basic blocks involved in critical edge splitting

[yozhu]

Fix an issue in #97618 - if the two basic blocks involved are not predecessor / successor to each other, treat the candidate as illegal for critical edge splitting.

Closes #98477 (checked in test copied from its comment).

[yozhu]

In one of our internal applications where this problem results in an assertion failure in MachineBasicBlock::replaceSuccessor(), there are a lots of DeferredFromBB discovered and only one of them is not immediate predecessor of the ToBB.

[mshockwave]

Could you put this check into isLegalToBreakCriticalEdge rather than the current place, since any pair of FromBB & ToBB is subject to this issue, not just the deferred one?

Also, please add tests.

[dtcxzyw]

Also, please add tests.

See #98477 (comment).

[alexfh]

Also, please add tests.

See #98477 (comment).

Also #97618 (comment) ;)

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: None (yozhu)

Changes

Fix an issue in #97618 - only sink candidate from immediate predecessor block should be added as candidate for critical edge splitting.

Closes #98477.

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/MachineSink.cpp (+1-1)
• (added) llvm/test/CodeGen/X86/MachineSink-Issue98477.ll (+36)

diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp
index bbc5ab13a0cd3..57b71d1a26859 100644
--- a/llvm/lib/CodeGen/MachineSink.cpp
+++ b/llvm/lib/CodeGen/MachineSink.cpp
@@ -959,7 +959,7 @@ bool MachineSinking::isLegalToBreakCriticalEdge(MachineInstr &MI,
                                                 MachineBasicBlock *ToBB,
                                                 bool BreakPHIEdge) {
   // Avoid breaking back edge. From == To means backedge for single BB cycle.
-  if (!SplitEdges || FromBB == ToBB)
+  if (!SplitEdges || FromBB == ToBB || !FromBB->isSuccessor(ToBB))
     return false;
 
   MachineCycle *FromCycle = CI->getCycle(FromBB);
diff --git a/llvm/test/CodeGen/X86/MachineSink-Issue98477.ll b/llvm/test/CodeGen/X86/MachineSink-Issue98477.ll
new file mode 100644
index 0000000000000..abc41341043c8
--- /dev/null
+++ b/llvm/test/CodeGen/X86/MachineSink-Issue98477.ll
@@ -0,0 +1,36 @@
+; RUN: llc < %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @main(i1 %tobool.not, i32 %0) {
+entry:
+  br i1 %tobool.not, label %if.end13, label %j.preheader
+
+  j.preheader:       ; preds = %if.end13, %entry
+  %h.0.ph = phi i32 [ 1, %entry ], [ 0, %if.end13 ]
+  br label %j
+
+  j:                 ; preds = %if.then4, %j.preheader
+  %1 = phi i32 [ %div2, %if.then4 ], [ 0, %j.preheader ]
+  %rem1 = srem i32 1, %0
+  %cmp = icmp slt i32 %1, 0
+  %or.cond = select i1 false, i1 true, i1 %cmp
+  br i1 %or.cond, label %if.then4, label %if.end9
+
+  if.then4:          ; preds = %j
+  %div2 = sdiv i32 1, 0
+  %rem5 = srem i32 1, %h.0.ph
+  br i1 %tobool.not, label %if.end9, label %j
+
+  if.end9:           ; preds = %if.then4, %j
+  %2 = phi i32 [ 0, %j ], [ %rem5, %if.then4 ]
+  %tobool10.not = icmp eq i32 %2, 0
+  br i1 %tobool10.not, label %if.end13, label %while.body.lr.ph
+
+  while.body.lr.ph:  ; preds = %if.end9
+  ret i32 %rem1
+
+  if.end13:          ; preds = %if.end9, %entry
+  br label %j.preheader
+}

[mshockwave]

Despite the fact that this test is checking whether llc crashes, I would prefer using FileCheck + UTC (Update Test Check) to check against the output just like other codegen tests, for the sake of uniformity.

[yozhu]

Updated the test.

[yozhu]

Also, please add tests.

See #98477 (comment).

Also #97618 (comment) ;)

Thanks @dtcxzyw and @alexfh :)

[alexfh]

Thanks for the fix!

[mshockwave]

LGTM thanks

[alexfh]

I'd like to get us unblocked before the weekend. I hope you won't mind me merging this PR.

[yozhu]

I'd like to get us unblocked before the weekend. I hope you won't mind me merging this PR.

Oh, not at all; and thanks for merging this PR :)

[yozhu]

LGTM thanks

Thanks for the review!