[llvm] [MacroFusion] Early return when insts already clustered (PR #191710)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-backend-aarch64

Author: Tomer Shafir (tomershafir)

<details>
<summary>Changes</summary>

This patch adds an early return to `fuseInstructionPair()` when macro fused instructions are already clustered, either by an earlier fusion or another clustering like ld/st clustering, removing the assert.
    
The assert is generally wrong - there are edge cases where an earlier ld/st clustering (before macro fusion) reached the assert because it sets `ParentClusterIdx` and fails. For example, ADRP+LOAD/STORE on AArch64, thought it seems to be a rare case because the addresses are ususally unkown at compile time.
    
It doesn't effectively change how fusions are prioritized - early fusions still win on fusion-fusion conflicts, like before. But it changes how we resolve the edge case of ld/st-fusion conflicts:
    
Previously, fusions would effectively override ld/st clustering in this case, given that we currently limits instruction membership to at most a single cluster through `ParentClusterIdx`. Macro fusion runs after ld/st clustering in the pipelines.
    
Here we inverse the priorities from MacroFusion's perspective and prefer earlier ld/st clustering. I think they should be generally preferred over fusions, because they not only save dispatch and execution slots, but they also should decrease code size. Of course this heuristic can fail in some cases. If a new clustering algorithm is added, it would have to inspect ordering and be placed correctly.
    
 I have inspected several internal workloads, and ld/st-fusion conflicts very rarely appear.

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


2 Files Affected:

- (modified) llvm/lib/CodeGen/MacroFusion.cpp (+12-16) 
- (added) llvm/test/CodeGen/AArch64/macro-fusion-cluster-conflict.mir (+75) 


``````````diff
diff --git a/llvm/lib/CodeGen/MacroFusion.cpp b/llvm/lib/CodeGen/MacroFusion.cpp
index 1db53017e6cef..55dadea676dbb 100644
--- a/llvm/lib/CodeGen/MacroFusion.cpp
+++ b/llvm/lib/CodeGen/MacroFusion.cpp
@@ -25,6 +25,9 @@
 #define DEBUG_TYPE "machine-scheduler"
 
 STATISTIC(NumFused, "Number of instr pairs fused");
+STATISTIC(NumFusionConflicts,
+          "Number of conflicts between a fusion pair and an already existing "
+          "cluster (either fusion or non-fusion)");
 
 using namespace llvm;
 
@@ -52,22 +55,15 @@ bool llvm::hasLessThanNumFused(const SUnit &SU, unsigned FuseLimit) {
 
 bool llvm::fuseInstructionPair(ScheduleDAGInstrs &DAG, SUnit &FirstSU,
                                SUnit &SecondSU) {
-  // Check that neither instr is already paired with another along the edge
-  // between them.
-  for (SDep &SI : FirstSU.Succs)
-    if (SI.isCluster())
-      return false;
-
-  for (SDep &SI : SecondSU.Preds)
-    if (SI.isCluster())
-      return false;
-
-  assert(FirstSU.ParentClusterIdx == InvalidClusterId &&
-         SecondSU.ParentClusterIdx == InvalidClusterId);
-
-  // Though the reachability checks above could be made more generic,
-  // perhaps as part of ScheduleDAGInstrs::addEdge(), since such edges are valid,
-  // the extra computation cost makes it less interesting in general cases.
+  // Check that neither instr is already associated with a cluster (either
+  // fusion or non-fusion)
+  if (FirstSU.isClustered() || SecondSU.isClustered()) {
+    ++NumFusionConflicts;
+    LLVM_DEBUG(dbgs() << "Fusion conflict: cannot fuse SU(" << FirstSU.NodeNum
+                      << ") and SU(" << SecondSU.NodeNum
+                      << ") - already clustered\n");
+    return false;
+  }
 
   // Create a single weak edge between the adjacent instrs. The only effect is
   // to cause bottom-up scheduling to heavily prioritize the clustered instrs.
diff --git a/llvm/test/CodeGen/AArch64/macro-fusion-cluster-conflict.mir b/llvm/test/CodeGen/AArch64/macro-fusion-cluster-conflict.mir
new file mode 100644
index 0000000000000..5dd3a3c9c3e28
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/macro-fusion-cluster-conflict.mir
@@ -0,0 +1,75 @@
+# REQUIRES: aarch64-registered-target
+# REQUIRES: asserts
+
+# RUN: llc -o /dev/null %s -mtriple=aarch64 -mattr=+fuse-address \
+# RUN:   -run-pass machine-scheduler -debug-only=machine-scheduler 2>&1 \
+# RUN:   | FileCheck %s
+# RUN: llc -o /dev/null %s -mtriple=aarch64-apple-macos -mcpu=apple-m5 \
+# RUN:   -run-pass machine-scheduler -debug-only=machine-scheduler 2>&1 \
+# RUN:   | FileCheck %s
+
+# Test macro fusion conflicts with other macro fusions and with ld/st clustering.
+
+# Fusion-fusion conflict: ADRP fans out to two loads at non-adjacent offsets
+# (so they are NOT clustered by LoadClusterDAGMutation). Both ADRP→LDR pairs
+# are valid address fusion pairs. SU(0)→SU(1) fuses first; when SU(0)→SU(2)
+# is attempted, SU(0) is already clustered.
+---
+name: fusion-fusion-conflict
+tracksRegLiveness: true
+body: |
+  bb.0:
+    ; CHECK-LABEL: fusion-fusion-conflict
+    ; CHECK: Macro fuse: SU(0) - SU(1) /  ADRP - LDRXui
+    ; CHECK: Fusion conflict: cannot fuse SU(0) and SU(2) - already clustered
+    ; CHECK: SU(0): $x0 = ADRP 0
+    ; CHECK: SU(1): $x1 = LDRXui $x0, 5
+    ; CHECK: SU(2): $x2 = LDRXui $x0, 10
+    $x0 = ADRP 0
+    $x1 = LDRXui $x0, 5 :: (load (s64))
+    $x2 = LDRXui $x0, 10 :: (load (s64))
+    RET_ReallyLR
+...
+
+# Store-fusion conflict: two adjacent stores get clustered by
+# StoreClusterDAGMutation (runs before AArch64 macro fusion). Then address
+# fusion (ADRP + STRXui) is attempted but the store SUnit is already
+# clustered.
+---
+name: store-fusion-conflict
+tracksRegLiveness: true
+body: |
+  bb.0:
+    liveins: $x0
+
+    ; CHECK-LABEL: store-fusion-conflict
+    ; CHECK: Cluster ld/st SU(1) - SU(2)
+    ; CHECK: Fusion conflict: cannot fuse SU(0) and SU(1) - already clustered
+    ; CHECK: SU(0): $x1 = ADRP 0
+    ; CHECK: SU(1): STRXui $x0, $x1, 0 :: (store (s64))
+    ; CHECK: SU(2): STRXui $x0, $x1, 1 :: (store (s64))
+    $x1 = ADRP 0
+    STRXui $x0, $x1, 0 :: (store (s64))
+    STRXui $x0, $x1, 1 :: (store (s64))
+    RET_ReallyLR
+...
+
+# Load-fusion conflict: two adjacent loads get clustered by
+# LoadClusterDAGMutation. Then address fusion (ADRP + LDRXui) is attempted
+# but the load SUnit is already clustered.
+---
+name: load-fusion-conflict
+tracksRegLiveness: true
+body: |
+  bb.0:
+    ; CHECK-LABEL: load-fusion-conflict
+    ; CHECK: Cluster ld/st SU(1) - SU(2)
+    ; CHECK: Fusion conflict: cannot fuse SU(0) and SU(1) - already clustered
+    ; CHECK: SU(0): $x0 = ADRP 0
+    ; CHECK: SU(1): $x1 = LDRXui $x0, 0
+    ; CHECK: SU(2): $x2 = LDRXui $x0, 1
+    $x0 = ADRP 0
+    $x1 = LDRXui $x0, 0 :: (load (s64))
+    $x2 = LDRXui $x0, 1 :: (load (s64))
+    RET_ReallyLR
+...

``````````

</details>


https://github.com/llvm/llvm-project/pull/191710