[MachinePipeliner] Detect a cycle in PHI dependencies early on #167095

quic-asaravan · 2025-11-08T05:02:06Z

This patch detects cycles by phis and bails out if one is found.
It prevents to violate DAG restrictions.

Abort pipelining in the below case

%1 = phi i32 [ %a, %entry ], [ %3, %loop ]
%2 = phi i32 [ %a, %entry ], [ %1, %loop ]
%3 = phi i32 [ %b, %entry ], [ %2, %loop ]

Abort pipelining in the below case %1 = phi i32 [ %a, %entry ], [ %3, %loop ] %2 = phi i32 [ %a, %entry ], [ %1, %loop ] %3 = phi i32 [ %b, %entry ], [ %2, %loop ]

llvmbot · 2025-11-08T05:02:48Z

@llvm/pr-subscribers-backend-hexagon

Author: Abinaya Saravanan (quic-asaravan)

Changes

Abort pipelining in the below case

%1 = phi i32 [ %a, %entry ], [ %3, %loop ]
%2 = phi i32 [ %a, %entry ], [ %1, %loop ]
%3 = phi i32 [ %b, %entry ], [ %2, %loop ]

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

2 Files Affected:

(modified) llvm/lib/CodeGen/MachinePipeliner.cpp (+54)
(added) llvm/test/CodeGen/Hexagon/swp-phi-cycle.ll (+22)

diff --git a/llvm/lib/CodeGen/MachinePipeliner.cpp b/llvm/lib/CodeGen/MachinePipeliner.cpp
index a717d9e4a618d..b327ba8900f03 100644
--- a/llvm/lib/CodeGen/MachinePipeliner.cpp
+++ b/llvm/lib/CodeGen/MachinePipeliner.cpp
@@ -485,6 +485,55 @@ void MachinePipeliner::setPragmaPipelineOptions(MachineLoop &L) {
   }
 }
 
+bool hasPHICycle(const MachineBasicBlock *LoopHeader, const MachineRegisterInfo &MRI) {
+  DenseMap<unsigned, SmallVector<unsigned, 2>> PhiDeps;
+  SmallSet<unsigned, 8> PhiRegs;
+
+  // Collect PHI nodes and their dependencies
+  for (const MachineInstr &MI : *LoopHeader) {
+    if (!MI.isPHI())
+      continue;
+
+    unsigned DefReg = MI.getOperand(0).getReg();
+    PhiRegs.insert(DefReg);
+
+    for (unsigned i = 1; i < MI.getNumOperands(); i += 2) {
+      unsigned SrcReg = MI.getOperand(i).getReg();
+      PhiDeps[DefReg].push_back(SrcReg);
+    }
+  }
+
+  // DFS to detect cycles
+  SmallSet<unsigned, 8> Visited, RecStack;
+
+  std::function<bool(unsigned)> DFS = [&](unsigned Reg) -> bool {
+  if (!PhiRegs.count(Reg))
+      return false;
+    if (RecStack.count(Reg))
+      return true;
+    if (Visited.count(Reg))
+      return false;
+
+    Visited.insert(Reg);
+    RecStack.insert(Reg);
+
+    for (unsigned Dep : PhiDeps[Reg]) {
+      if (DFS(Dep))
+        return true;
+    }
+
+    RecStack.erase(Reg);
+    return false;
+  };
+
+  for (unsigned Reg : PhiRegs) {
+    if (DFS(Reg))
+      return true;
+  }
+
+  return false;
+}
+
 /// Return true if the loop can be software pipelined.  The algorithm is
 /// restricted to loops with a single basic block.  Make sure that the
 /// branch in the loop can be analyzed.
@@ -499,6 +548,11 @@ bool MachinePipeliner::canPipelineLoop(MachineLoop &L) {
     return false;
   }
 
+  if (hasPHICycle(L.getHeader(), MF->getRegInfo())) {
+    LLVM_DEBUG(dbgs() << "Cannot pipeline loop due to PHI cycle\n");
+    return false;
+  }
+
   if (disabledByPragma) {
     ORE->emit([&]() {
       return MachineOptimizationRemarkAnalysis(DEBUG_TYPE, "canPipelineLoop",
diff --git a/llvm/test/CodeGen/Hexagon/swp-phi-cycle.ll b/llvm/test/CodeGen/Hexagon/swp-phi-cycle.ll
new file mode 100644
index 0000000000000..a92d113f01c88
--- /dev/null
+++ b/llvm/test/CodeGen/Hexagon/swp-phi-cycle.ll
@@ -0,0 +1,22 @@
+; RUN: llc -mtriple=hexagon-unknown-linux-gnu -enable-pipeliner -debug-only=pipeliner < %s 2>&1 | FileCheck %s
+
+; CHECK: Cannot pipeline loop due to PHI cycle
+
+define void @phi_cycle_loop(i32 %a, i32 %b) {
+entry:
+  br label %loop
+
+loop:
+  %1 = phi i32 [ %a, %entry ], [ %3, %loop ]
+  %2 = phi i32 [ %a, %entry ], [ %1, %loop ]
+  %3 = phi i32 [ %b, %entry ], [ %2, %loop ]
+
+  ; Prevent PHI elimination by using all values
+  %add1 = add i32 %1, %2
+  %add2 = add i32 %add1, %3
+  %cmp = icmp slt i32 %add2, 100
+  br i1 %cmp, label %loop, label %exit
+
+exit:
+  ret void
+}

github-actions · 2025-11-08T05:03:44Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Abort pipelining in the below case %1 = phi i32 [ %a, %entry ], [ %3, %loop ] %2 = phi i32 [ %a, %entry ], [ %1, %loop ] %3 = phi i32 [ %b, %entry ], [ %2, %loop ]

…ravan/llvm-project into Special-case-PHIs_cycle

quic-asaravan · 2025-11-09T05:15:47Z

@iajbar @sgundapa Could you please review ?

quic-asaravan · 2025-11-11T08:09:13Z

@kasuga-fj Could you please review this patch?

kasuga-fj

Generally looks reasonable to me.

llvm/lib/CodeGen/MachinePipeliner.cpp

kasuga-fj · 2025-11-11T09:47:26Z

llvm/lib/CodeGen/MachinePipeliner.cpp

+  // DFS to detect cycles
+  SmallSet<unsigned, 8> Visited, RecStack;
+
+  std::function<bool(unsigned)> DFS = [&](unsigned Reg) -> bool {


I'd prefer to define a function like static bool foo(unsigned Reg, ...) rather than using std::function for recursive function.

kasuga-fj · 2025-11-11T10:05:20Z

llvm/lib/CodeGen/MachinePipeliner.cpp

+    PhiRegs.insert(DefReg);
+
+    for (unsigned i = 1; i < MI.getNumOperands(); i += 2) {
+      unsigned SrcReg = MI.getOperand(i).getReg();
+      PhiDeps[DefReg].push_back(SrcReg);
+    }


Suggested change

PhiRegs.insert(DefReg);

for (unsigned i = 1; i < MI.getNumOperands(); i += 2) {

unsigned SrcReg = MI.getOperand(i).getReg();

PhiDeps[DefReg].push_back(SrcReg);

}

auto Ite = PhiDeps.try_emplace(DefReg).first;

for (unsigned i = 1; i < MI.getNumOperands(); i += 2)

Ite->second.push_back(MI.getOperand(i).getReg());

To avoid repeated hash lookup.

kasuga-fj · 2025-11-11T10:06:37Z

llvm/lib/CodeGen/MachinePipeliner.cpp

+    if (!PhiRegs.count(Reg))
+      return false;


Consider calling PhiDeps.find(Reg) instead and remove PhiRegs.

llvm/test/CodeGen/Hexagon/swp-phi-cycle.ll

llvm/lib/CodeGen/MachinePipeliner.cpp

Co-authored-by: Ryotaro Kasuga <[email protected]>

quic-asaravan · 2025-11-12T16:08:07Z

@kasuga-fj Could you please review? I have made the suggested changes.

kasuga-fj

The code looks good to me, but I think the PR description should be updated. At least the following should be included.

This patch detects cycles by phis and bails out if one is found.
It prevents to violate DAG restrictions.

llvm/lib/CodeGen/MachinePipeliner.cpp

Co-authored-by: Ryotaro Kasuga <[email protected]>

llvm-ci · 2025-11-17T10:13:21Z

LLVM Buildbot has detected a new failure on builder llvm-nvptx-nvidia-win running on as-builder-8 while building llvm at step 7 "test-build-unified-tree-check-llvm".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/54/builds/14754

Here is the relevant piece of the build log for the reference

Step 7 (test-build-unified-tree-check-llvm) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/79/105' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\buildbot\as-builder-8\llvm-nvptx-nvidia-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-19284-79-105.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=105 GTEST_SHARD_INDEX=79 C:\buildbot\as-builder-8\llvm-nvptx-nvidia-win\build\unittests\Support\.\SupportTests.exe
--


Note: This is test shard 80 of 105.

[==========] Running 16 tests from 16 test suites.

[----------] Global test environment set-up.

[----------] 1 test from BinaryStreamTest

[ RUN      ] BinaryStreamTest.DropOperations

[       OK ] BinaryStreamTest.DropOperations (0 ms)

[----------] 1 test from BinaryStreamTest (0 ms total)



[----------] 1 test from CommandLineTest

[ RUN      ] CommandLineTest.TokenizeAndMarkEOLs

[       OK ] CommandLineTest.TokenizeAndMarkEOLs (0 ms)

[----------] 1 test from CommandLineTest (0 ms total)



[----------] 1 test from DataExtractorTest

[ RUN      ] DataExtractorTest.LEB128_error

[       OK ] DataExtractorTest.LEB128_error (0 ms)

[----------] 1 test from DataExtractorTest (0 ms total)



[----------] 1 test from Error

[ RUN      ] Error.ForwardToExpected

[       OK ] Error.ForwardToExpected (0 ms)

[----------] 1 test from Error (0 ms total)
...

handled early on - llvm#167095

…167095) - This patch detects cycles by phis and bails out if one is found. - It prevents to violate DAG restrictions. Abort pipelining in the below case %1 = phi i32 [ %a, %entry ], [ %3, %loop ] %2 = phi i32 [ %a, %entry ], [ %1, %loop ] %3 = phi i32 [ %b, %entry ], [ %2, %loop ] --------- Co-authored-by: Ryotaro Kasuga <[email protected]>

[MachinePipeliner] Detect a cycle in PHI dependencies early on

06c013f

Abort pipelining in the below case %1 = phi i32 [ %a, %entry ], [ %3, %loop ] %2 = phi i32 [ %a, %entry ], [ %1, %loop ] %3 = phi i32 [ %b, %entry ], [ %2, %loop ]

quic-asaravan requested a review from aankit-ca November 8, 2025 05:02

llvmbot added backend:Hexagon llvm:codegen labels Nov 8, 2025

quic-asaravan marked this pull request as draft November 8, 2025 05:02

quic-asaravan self-assigned this Nov 8, 2025

quic-asaravan added 2 commits November 8, 2025 05:28

[MachinePipeliner] Detect a cycle in PHI dependencies early on

68a3d5f

Abort pipelining in the below case %1 = phi i32 [ %a, %entry ], [ %3, %loop ] %2 = phi i32 [ %a, %entry ], [ %1, %loop ] %3 = phi i32 [ %b, %entry ], [ %2, %loop ]

Merge branch 'Special-case-PHIs_cycle' of https://github.com/quic-asa…

c2e7487

…ravan/llvm-project into Special-case-PHIs_cycle

quic-asaravan marked this pull request as ready for review November 9, 2025 05:11

quic-asaravan mentioned this pull request Nov 9, 2025

[HEXAGON] [MachinePipeliner] Fix the DAG in case of dependent phis. #135925

Open

iajbar self-requested a review November 10, 2025 15:10

kasuga-fj reviewed Nov 11, 2025

View reviewed changes

quic-asaravan and others added 5 commits November 11, 2025 15:50

Update llvm/lib/CodeGen/MachinePipeliner.cpp

1060047

Co-authored-by: Ryotaro Kasuga <[email protected]>

Update llvm/test/CodeGen/Hexagon/swp-phi-cycle.ll

4354dd4

Co-authored-by: Ryotaro Kasuga <[email protected]>

Update llvm/lib/CodeGen/MachinePipeliner.cpp

10698b7

Co-authored-by: Ryotaro Kasuga <[email protected]>

Taking review suggestions

f7084ce

Running clang format

0d0d3e1

kasuga-fj approved these changes Nov 14, 2025

View reviewed changes

llvm/lib/CodeGen/MachinePipeliner.cpp Outdated Show resolved Hide resolved

quic-asaravan and others added 2 commits November 17, 2025 13:12

Update llvm/lib/CodeGen/MachinePipeliner.cpp

9bac48e

Co-authored-by: Ryotaro Kasuga <[email protected]>

Merge branch 'main' into Special-case-PHIs_cycle

a282ca4

quic-asaravan merged commit c946418 into llvm:main Nov 17, 2025
10 checks passed

quic-asaravan added a commit to quic-asaravan/llvm-project that referenced this pull request Nov 17, 2025

Removing reachability check as PHI-PHI dependencies cycles are

49243dc

handled early on - llvm#167095

[MachinePipeliner] Detect a cycle in PHI dependencies early on #167095

[MachinePipeliner] Detect a cycle in PHI dependencies early on #167095

Uh oh!

Conversation

quic-asaravan commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 8, 2025

Uh oh!

github-actions bot commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quic-asaravan commented Nov 9, 2025

Uh oh!

quic-asaravan commented Nov 11, 2025

Uh oh!

kasuga-fj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kasuga-fj Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

kasuga-fj Nov 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

quic-asaravan commented Nov 12, 2025

Uh oh!

kasuga-fj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvm-ci commented Nov 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

quic-asaravan commented Nov 8, 2025 •

edited

Loading

github-actions bot commented Nov 8, 2025 •

edited

Loading