Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NFC] Refactor looping over recomputeLiveIns into function #88040

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

redstar
Copy link
Member

@redstar redstar commented Apr 8, 2024

#79940 put calls to recomputeLiveIns into
a loop, to repeatedly call the function until the computation converges. However,
this repeats a lot of code. This changes moves the loop into a function to simplify
the handling.

Note that this changes the order in which recomputeLiveIns is called. For example,

  bool anyChange = false;
  do {
    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
  } while (anyChange);

only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB
has converged. With this change, all basic blocks have a recomputation of the live-ins
for each loop iteration. This can result in less or more calls, depending on the
situation.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 8, 2024

@llvm/pr-subscribers-backend-x86
@llvm/pr-subscribers-backend-systemz

@llvm/pr-subscribers-backend-powerpc

Author: Kai Nacke (redstar)

Changes

#79940 put calls to recomputeLiveIns into
a loop, to repeatedly call the function until the computation converges. However,
this repeats a lot of code. This changes moves the loop into a function to simplify
the handling.

Note that this changes the order in which recomputeLiveIns is called. For example,

  bool anyChange = false;
  do {
    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
  } while (anyChange);

only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB
has converged. With this change, all basic blocks have a recomputation of the live-ins
for each loop iteration. This can result in less or more calls, depending on the
situation.


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

9 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/LivePhysRegs.h (+14)
  • (modified) llvm/lib/CodeGen/BranchFolding.cpp (+2-6)
  • (modified) llvm/lib/Target/AArch64/AArch64FrameLowering.cpp (+1-4)
  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.cpp (+2-9)
  • (modified) llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp (+1-7)
  • (modified) llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp (+2-9)
  • (modified) llvm/lib/Target/PowerPC/PPCFrameLowering.cpp (+2-9)
  • (modified) llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp (+2-8)
  • (modified) llvm/lib/Target/X86/X86FrameLowering.cpp (+2-9)
diff --git a/llvm/include/llvm/CodeGen/LivePhysRegs.h b/llvm/include/llvm/CodeGen/LivePhysRegs.h
index 1d40b1cbb0eaa3..a2447e7702c77f 100644
--- a/llvm/include/llvm/CodeGen/LivePhysRegs.h
+++ b/llvm/include/llvm/CodeGen/LivePhysRegs.h
@@ -39,6 +39,8 @@
 
 namespace llvm {
 
+template <typename T> class ArrayRef;
+
 class MachineInstr;
 class MachineFunction;
 class MachineOperand;
@@ -207,6 +209,18 @@ static inline bool recomputeLiveIns(MachineBasicBlock &MBB) {
   return oldLiveIns != newLiveIns;
 }
 
+/// Convenience function for recomputing live-in's for a set of MBBs until the
+/// computation converges.
+static inline void
+fullyRecomputeLiveIns(ArrayRef<MachineBasicBlock *> MBBs) {
+  bool AnyChange;
+  do {
+    AnyChange = false;
+    for (MachineBasicBlock *MBB : MBBs)
+      AnyChange = recomputeLiveIns(*MBB) || AnyChange;
+  } while (AnyChange);
+}
+
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_LIVEPHYSREGS_H
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index ecf7bc30913f51..55aa1d438b2a66 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2047,12 +2047,8 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
   MBB->splice(Loc, TBB, TBB->begin(), TIB);
   FBB->erase(FBB->begin(), FIB);
 
-  if (UpdateLiveIns) {
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*TBB) || recomputeLiveIns(*FBB);
-    } while (anyChange);
-  }
+  if (UpdateLiveIns)
+    fullyRecomputeLiveIns({TBB, FBB});
 
   ++NumHoist;
   return true;
diff --git a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
index 5cc612e89162af..419c141121c325 100644
--- a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
@@ -4325,10 +4325,7 @@ AArch64FrameLowering::inlineStackProbeLoopExactMultiple(
   ExitMBB->transferSuccessorsAndUpdatePHIs(&MBB);
   MBB.addSuccessor(LoopMBB);
   // Update liveins.
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({ExitMBB, LoopMBB});
 
   return ExitMBB->begin();
 }
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 22687b0e31c284..9f7176694d540a 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9557,15 +9557,8 @@ AArch64InstrInfo::probedStackAlloc(MachineBasicBlock::iterator MBBI,
   MBB.addSuccessor(LoopTestMBB);
 
   // Update liveins.
-  if (MF.getRegInfo().reservedRegsFrozen()) {
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*ExitMBB) ||
-                  recomputeLiveIns(*LoopBodyMBB) ||
-                  recomputeLiveIns(*LoopTestMBB);
-    } while (anyChange);
-    ;
-  }
+  if (MF.getRegInfo().reservedRegsFrozen())
+    fullyRecomputeLiveIns({ExitMBB, LoopBodyMBB, LoopTestMBB});
 
   return ExitMBB->begin();
 }
diff --git a/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp b/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
index 8629551152cb64..ea5dd5427ce720 100644
--- a/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
+++ b/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
@@ -1806,13 +1806,7 @@ void ARMLowOverheadLoops::Expand(LowOverheadLoop &LoLoop) {
   PostOrderLoopTraversal DFS(LoLoop.ML, *MLI);
   DFS.ProcessLoop();
   const SmallVectorImpl<MachineBasicBlock*> &PostOrder = DFS.getOrder();
-  bool anyChange = false;
-  do {
-    anyChange = false;
-    for (auto *MBB : PostOrder) {
-      anyChange = recomputeLiveIns(*MBB) || anyChange;
-    }
-  } while (anyChange);
+  fullyRecomputeLiveIns(PostOrder);
 
   for (auto *MBB : reverse(PostOrder))
     recomputeLivenessFlags(*MBB);
diff --git a/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp b/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
index b43eee8fdd8c0f..b3cfcb2aa14405 100644
--- a/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
+++ b/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
@@ -208,10 +208,7 @@ bool PPCExpandAtomicPseudo::expandAtomicRMW128(
       .addMBB(LoopMBB);
   CurrentMBB->addSuccessor(LoopMBB);
   CurrentMBB->addSuccessor(ExitMBB);
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({ExitMBB, LoopMBB});
   NMBBI = MBB.end();
   MI.eraseFromParent();
   return true;
@@ -288,11 +285,7 @@ bool PPCExpandAtomicPseudo::expandAtomicCmpSwap128(
   CurrentMBB->addSuccessor(LoopCmpMBB);
   CurrentMBB->addSuccessor(ExitMBB);
 
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*CmpSuccMBB) ||
-                recomputeLiveIns(*LoopCmpMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({ExitMBB, CmpSuccMBB, LoopCmpMBB});
   NMBBI = MBB.end();
   MI.eraseFromParent();
   return true;
diff --git a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
index 6dcb59a3a57f85..04e9f9e2366edd 100644
--- a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
@@ -1435,11 +1435,7 @@ void PPCFrameLowering::inlineStackProbe(MachineFunction &MF,
       ProbeLoopBodyMBB->addSuccessor(ProbeLoopBodyMBB);
     }
     // Update liveins.
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*ProbeExitMBB) ||
-                  recomputeLiveIns(*ProbeLoopBodyMBB);
-    } while (anyChange);
+    fullyRecomputeLiveIns({ProbeExitMBB, ProbeLoopBodyMBB});
     return ProbeExitMBB;
   };
   // For case HasBP && MaxAlign > 1, we have to realign the SP by performing
@@ -1531,10 +1527,7 @@ void PPCFrameLowering::inlineStackProbe(MachineFunction &MF,
         buildDefCFAReg(*ExitMBB, ExitMBB->begin(), SPReg);
       }
       // Update liveins.
-      bool anyChange = false;
-      do {
-        anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
-      } while (anyChange);
+      fullyRecomputeLiveIns({ExitMBB, LoopMBB});
     }
   }
   ++NumPrologProbed;
diff --git a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
index 4897b37d8eb1ef..50ecd6e0744147 100644
--- a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
@@ -824,10 +824,7 @@ void SystemZELFFrameLowering::inlineStackProbe(
   StackAllocMI->eraseFromParent();
   if (DoneMBB != nullptr) {
     // Compute the live-in lists for the new blocks.
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*DoneMBB) || recomputeLiveIns(*LoopMBB);
-    } while (anyChange);
+    fullyRecomputeLiveIns({DoneMBB, LoopMBB});
   }
 }
 
@@ -1425,10 +1422,7 @@ void SystemZXPLINKFrameLowering::inlineStackProbe(
   StackAllocMI->eraseFromParent();
 
   // Compute the live-in lists for the new blocks.
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*StackExtMBB) || recomputeLiveIns(*NextMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({StackExtMBB, NextMBB});
 }
 
 bool SystemZXPLINKFrameLowering::hasFP(const MachineFunction &MF) const {
diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index d914e1b61ab075..4521401d8741c7 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -885,10 +885,7 @@ void X86FrameLowering::emitStackProbeInlineGenericLoop(
   }
 
   // Update Live In information
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*tailMBB) || recomputeLiveIns(*testMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({tailMBB, testMBB});
 }
 
 void X86FrameLowering::emitStackProbeInlineWindowsCoreCLR64(
@@ -1380,11 +1377,7 @@ void X86FrameLowering::BuildStackAlignAND(MachineBasicBlock &MBB,
         footMBB->addSuccessor(&MBB);
       }
 
-      bool anyChange = false;
-      do {
-        anyChange = recomputeLiveIns(*footMBB) || recomputeLiveIns(*bodyMBB) ||
-                    recomputeLiveIns(*headMBB) || recomputeLiveIns(MBB);
-      } while (anyChange);
+      fullyRecomputeLiveIns({footMBB, bodyMBB, headMBB, &MBB});
     }
   } else {
     MachineInstr *MI = BuildMI(MBB, MBBI, DL, TII.get(AndOp), Reg)

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 8, 2024

@llvm/pr-subscribers-backend-arm

Author: Kai Nacke (redstar)

Changes

#79940 put calls to recomputeLiveIns into
a loop, to repeatedly call the function until the computation converges. However,
this repeats a lot of code. This changes moves the loop into a function to simplify
the handling.

Note that this changes the order in which recomputeLiveIns is called. For example,

  bool anyChange = false;
  do {
    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
  } while (anyChange);

only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB
has converged. With this change, all basic blocks have a recomputation of the live-ins
for each loop iteration. This can result in less or more calls, depending on the
situation.


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

9 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/LivePhysRegs.h (+14)
  • (modified) llvm/lib/CodeGen/BranchFolding.cpp (+2-6)
  • (modified) llvm/lib/Target/AArch64/AArch64FrameLowering.cpp (+1-4)
  • (modified) llvm/lib/Target/AArch64/AArch64InstrInfo.cpp (+2-9)
  • (modified) llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp (+1-7)
  • (modified) llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp (+2-9)
  • (modified) llvm/lib/Target/PowerPC/PPCFrameLowering.cpp (+2-9)
  • (modified) llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp (+2-8)
  • (modified) llvm/lib/Target/X86/X86FrameLowering.cpp (+2-9)
diff --git a/llvm/include/llvm/CodeGen/LivePhysRegs.h b/llvm/include/llvm/CodeGen/LivePhysRegs.h
index 1d40b1cbb0eaa3..a2447e7702c77f 100644
--- a/llvm/include/llvm/CodeGen/LivePhysRegs.h
+++ b/llvm/include/llvm/CodeGen/LivePhysRegs.h
@@ -39,6 +39,8 @@
 
 namespace llvm {
 
+template <typename T> class ArrayRef;
+
 class MachineInstr;
 class MachineFunction;
 class MachineOperand;
@@ -207,6 +209,18 @@ static inline bool recomputeLiveIns(MachineBasicBlock &MBB) {
   return oldLiveIns != newLiveIns;
 }
 
+/// Convenience function for recomputing live-in's for a set of MBBs until the
+/// computation converges.
+static inline void
+fullyRecomputeLiveIns(ArrayRef<MachineBasicBlock *> MBBs) {
+  bool AnyChange;
+  do {
+    AnyChange = false;
+    for (MachineBasicBlock *MBB : MBBs)
+      AnyChange = recomputeLiveIns(*MBB) || AnyChange;
+  } while (AnyChange);
+}
+
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_LIVEPHYSREGS_H
diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp
index ecf7bc30913f51..55aa1d438b2a66 100644
--- a/llvm/lib/CodeGen/BranchFolding.cpp
+++ b/llvm/lib/CodeGen/BranchFolding.cpp
@@ -2047,12 +2047,8 @@ bool BranchFolder::HoistCommonCodeInSuccs(MachineBasicBlock *MBB) {
   MBB->splice(Loc, TBB, TBB->begin(), TIB);
   FBB->erase(FBB->begin(), FIB);
 
-  if (UpdateLiveIns) {
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*TBB) || recomputeLiveIns(*FBB);
-    } while (anyChange);
-  }
+  if (UpdateLiveIns)
+    fullyRecomputeLiveIns({TBB, FBB});
 
   ++NumHoist;
   return true;
diff --git a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
index 5cc612e89162af..419c141121c325 100644
--- a/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
@@ -4325,10 +4325,7 @@ AArch64FrameLowering::inlineStackProbeLoopExactMultiple(
   ExitMBB->transferSuccessorsAndUpdatePHIs(&MBB);
   MBB.addSuccessor(LoopMBB);
   // Update liveins.
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({ExitMBB, LoopMBB});
 
   return ExitMBB->begin();
 }
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 22687b0e31c284..9f7176694d540a 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -9557,15 +9557,8 @@ AArch64InstrInfo::probedStackAlloc(MachineBasicBlock::iterator MBBI,
   MBB.addSuccessor(LoopTestMBB);
 
   // Update liveins.
-  if (MF.getRegInfo().reservedRegsFrozen()) {
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*ExitMBB) ||
-                  recomputeLiveIns(*LoopBodyMBB) ||
-                  recomputeLiveIns(*LoopTestMBB);
-    } while (anyChange);
-    ;
-  }
+  if (MF.getRegInfo().reservedRegsFrozen())
+    fullyRecomputeLiveIns({ExitMBB, LoopBodyMBB, LoopTestMBB});
 
   return ExitMBB->begin();
 }
diff --git a/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp b/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
index 8629551152cb64..ea5dd5427ce720 100644
--- a/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
+++ b/llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp
@@ -1806,13 +1806,7 @@ void ARMLowOverheadLoops::Expand(LowOverheadLoop &LoLoop) {
   PostOrderLoopTraversal DFS(LoLoop.ML, *MLI);
   DFS.ProcessLoop();
   const SmallVectorImpl<MachineBasicBlock*> &PostOrder = DFS.getOrder();
-  bool anyChange = false;
-  do {
-    anyChange = false;
-    for (auto *MBB : PostOrder) {
-      anyChange = recomputeLiveIns(*MBB) || anyChange;
-    }
-  } while (anyChange);
+  fullyRecomputeLiveIns(PostOrder);
 
   for (auto *MBB : reverse(PostOrder))
     recomputeLivenessFlags(*MBB);
diff --git a/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp b/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
index b43eee8fdd8c0f..b3cfcb2aa14405 100644
--- a/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
+++ b/llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp
@@ -208,10 +208,7 @@ bool PPCExpandAtomicPseudo::expandAtomicRMW128(
       .addMBB(LoopMBB);
   CurrentMBB->addSuccessor(LoopMBB);
   CurrentMBB->addSuccessor(ExitMBB);
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({ExitMBB, LoopMBB});
   NMBBI = MBB.end();
   MI.eraseFromParent();
   return true;
@@ -288,11 +285,7 @@ bool PPCExpandAtomicPseudo::expandAtomicCmpSwap128(
   CurrentMBB->addSuccessor(LoopCmpMBB);
   CurrentMBB->addSuccessor(ExitMBB);
 
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*CmpSuccMBB) ||
-                recomputeLiveIns(*LoopCmpMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({ExitMBB, CmpSuccMBB, LoopCmpMBB});
   NMBBI = MBB.end();
   MI.eraseFromParent();
   return true;
diff --git a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
index 6dcb59a3a57f85..04e9f9e2366edd 100644
--- a/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
@@ -1435,11 +1435,7 @@ void PPCFrameLowering::inlineStackProbe(MachineFunction &MF,
       ProbeLoopBodyMBB->addSuccessor(ProbeLoopBodyMBB);
     }
     // Update liveins.
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*ProbeExitMBB) ||
-                  recomputeLiveIns(*ProbeLoopBodyMBB);
-    } while (anyChange);
+    fullyRecomputeLiveIns({ProbeExitMBB, ProbeLoopBodyMBB});
     return ProbeExitMBB;
   };
   // For case HasBP && MaxAlign > 1, we have to realign the SP by performing
@@ -1531,10 +1527,7 @@ void PPCFrameLowering::inlineStackProbe(MachineFunction &MF,
         buildDefCFAReg(*ExitMBB, ExitMBB->begin(), SPReg);
       }
       // Update liveins.
-      bool anyChange = false;
-      do {
-        anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
-      } while (anyChange);
+      fullyRecomputeLiveIns({ExitMBB, LoopMBB});
     }
   }
   ++NumPrologProbed;
diff --git a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
index 4897b37d8eb1ef..50ecd6e0744147 100644
--- a/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp
@@ -824,10 +824,7 @@ void SystemZELFFrameLowering::inlineStackProbe(
   StackAllocMI->eraseFromParent();
   if (DoneMBB != nullptr) {
     // Compute the live-in lists for the new blocks.
-    bool anyChange = false;
-    do {
-      anyChange = recomputeLiveIns(*DoneMBB) || recomputeLiveIns(*LoopMBB);
-    } while (anyChange);
+    fullyRecomputeLiveIns({DoneMBB, LoopMBB});
   }
 }
 
@@ -1425,10 +1422,7 @@ void SystemZXPLINKFrameLowering::inlineStackProbe(
   StackAllocMI->eraseFromParent();
 
   // Compute the live-in lists for the new blocks.
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*StackExtMBB) || recomputeLiveIns(*NextMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({StackExtMBB, NextMBB});
 }
 
 bool SystemZXPLINKFrameLowering::hasFP(const MachineFunction &MF) const {
diff --git a/llvm/lib/Target/X86/X86FrameLowering.cpp b/llvm/lib/Target/X86/X86FrameLowering.cpp
index d914e1b61ab075..4521401d8741c7 100644
--- a/llvm/lib/Target/X86/X86FrameLowering.cpp
+++ b/llvm/lib/Target/X86/X86FrameLowering.cpp
@@ -885,10 +885,7 @@ void X86FrameLowering::emitStackProbeInlineGenericLoop(
   }
 
   // Update Live In information
-  bool anyChange = false;
-  do {
-    anyChange = recomputeLiveIns(*tailMBB) || recomputeLiveIns(*testMBB);
-  } while (anyChange);
+  fullyRecomputeLiveIns({tailMBB, testMBB});
 }
 
 void X86FrameLowering::emitStackProbeInlineWindowsCoreCLR64(
@@ -1380,11 +1377,7 @@ void X86FrameLowering::BuildStackAlignAND(MachineBasicBlock &MBB,
         footMBB->addSuccessor(&MBB);
       }
 
-      bool anyChange = false;
-      do {
-        anyChange = recomputeLiveIns(*footMBB) || recomputeLiveIns(*bodyMBB) ||
-                    recomputeLiveIns(*headMBB) || recomputeLiveIns(MBB);
-      } while (anyChange);
+      fullyRecomputeLiveIns({footMBB, bodyMBB, headMBB, &MBB});
     }
   } else {
     MachineInstr *MI = BuildMI(MBB, MBBI, DL, TII.get(AndOp), Reg)

@redstar redstar requested review from oskarwirga and nikic and removed request for oskarwirga April 8, 2024 20:54
Copy link

github-actions bot commented Apr 8, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff a9111d4a26514e169ca57c11e6533ca7c5408832 c722f4728e13e22b4896131f5dc0bc8094c09494 -- llvm/include/llvm/CodeGen/LivePhysRegs.h llvm/lib/CodeGen/BranchFolding.cpp llvm/lib/Target/AArch64/AArch64FrameLowering.cpp llvm/lib/Target/AArch64/AArch64InstrInfo.cpp llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp llvm/lib/Target/PowerPC/PPCExpandAtomicPseudoInsts.cpp llvm/lib/Target/PowerPC/PPCFrameLowering.cpp llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp llvm/lib/Target/X86/X86FrameLowering.cpp
View the diff from clang-format here.
diff --git a/llvm/include/llvm/CodeGen/LivePhysRegs.h b/llvm/include/llvm/CodeGen/LivePhysRegs.h
index 9574a6f0c7..ec4ac5947f 100644
--- a/llvm/include/llvm/CodeGen/LivePhysRegs.h
+++ b/llvm/include/llvm/CodeGen/LivePhysRegs.h
@@ -224,7 +224,6 @@ inline void fullyRecomputeLiveIns(ArrayRef<MachineBasicBlock *> MBBs) {
   }
 }
 
-
 } // end namespace llvm
 
 #endif // LLVM_CODEGEN_LIVEPHYSREGS_H

@redstar
Copy link
Member Author

redstar commented Apr 8, 2024

My understanding is that the live-ins computation for the basic blocks is independent
of each other. If this is correct, then the loop could be formulated as follows, which avoids
unnecessary calls.

static inline void
fullyRecomputeLiveIns(ArrayRef<MachineBasicBlock *> MBBs) {
  for (MachineBasicBlock *MBB : MBBs)
    while (recomputeLiveIns(*MBB))
      /* intentionally empty/* ;
}

Also, is the function recomputeLiveIns in the current form needed? I see no callers to it, so both functions could be integrated into one.

@oskarwirga oskarwirga requested a review from yozhu April 8, 2024 21:15
@momchil-velikov
Copy link
Collaborator

My understanding is that the live-ins computation for the basic blocks is independent of each other.
Live-ins are computed by backward data-flow analysis, thus live-ins of a block depend on the live-outs of the block, which
in turn depend on the live-ins of the successor blocks.

If this is correct, then the loop could be formulated as follows, which avoids unnecessary calls.

static inline void
fullyRecomputeLiveIns(ArrayRef<MachineBasicBlock *> MBBs) {
  for (MachineBasicBlock *MBB : MBBs)
    while (recomputeLiveIns(*MBB))
      /* intentionally empty/* ;
}

That's what I expect to result in unnecessary calls since once you've computed live-ins for a block, immediately (i.e. without touching a different block) re-computing them for the same block would result in no change (50% waste), except when the block loops into itself, in which case only you get "only" 33.3% waste.

Also, is the function recomputeLiveIns in the current form needed? I see no callers to it, so both functions could be integrated into one.

I would suggest to leave it alone, so one can control independently the computation on a block and the order in which those computations are performed (even though DFS order is universally preferred).

Copy link
Collaborator

@momchil-velikov momchil-velikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Perhaps it's worth checking CTMark times, I'd improvement (perhaps not measurable overall), but just to avoid surprises.

@@ -207,6 +209,18 @@ static inline bool recomputeLiveIns(MachineBasicBlock &MBB) {
return oldLiveIns != newLiveIns;
}

/// Convenience function for recomputing live-in's for a set of MBBs until the
/// computation converges.
static inline void
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new function doesn't need to be defined as static in the header file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

@redstar
Copy link
Member Author

redstar commented Apr 9, 2024

LGTM. Perhaps it's worth checking CTMark times, I'd improvement (perhaps not measurable overall), but just to avoid surprises.

Good point. I'll try a run.

Copy link
Contributor

@oskarwirga oskarwirga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested these changes locally and they worked, LGTM!

llvm#79940 put calls to recomputeLiveIns into
a loop, to repeatedly call the function until the computation converges. However,
this repeats a lot of code. This changes moves the loop into a function to simplify
the handling.

Note that this changes the order in which recomputeLiveIns is called. For example,

```
  bool anyChange = false;
  do {
    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
  } while (anyChange);
```

only begins to recompute the live-ins for LoopMBB after the computation for ExitMBB
has converged. With this change, all basic blocks have a recomputation of the live-ins
for each loop iteration. This can result in less or more calls, depending on the
situation.
as suggested by review comment.
@redstar
Copy link
Member Author

redstar commented Apr 15, 2024

LGTM. Perhaps it's worth checking CTMark times, I'd improvement (perhaps not measurable overall), but just to avoid surprises.

Good point. I'll try a run.

Sorry for the delay, I ran into some trouble with CTMark. Turned out that the commit I based my changes on had a problem.
I see a small degradation in compile time performance, which surprises me since I also checked some of the test cases, and the number of calls to recomputeLiveIns() decreased. I like to understand this first before committing the change.

@redstar
Copy link
Member Author

redstar commented Apr 15, 2024

I removed all the syntactic sugar inside the loop. Now I get the expected result: with this change, clang is faster than the unmodified version (on CTMark).
Please have another look. Thanks.

@redstar redstar merged commit 21d1770 into llvm:main Apr 15, 2024
2 of 4 checks passed
aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 15, 2024
llvm#79940 put calls to
recomputeLiveIns into
a loop, to repeatedly call the function until the computation converges.
However,
this repeats a lot of code. This changes moves the loop into a function
to simplify
the handling.

Note that this changes the order in which recomputeLiveIns is called.
For example,

```
  bool anyChange = false;
  do {
    anyChange = recomputeLiveIns(*ExitMBB) || recomputeLiveIns(*LoopMBB);
  } while (anyChange);
```

only begins to recompute the live-ins for LoopMBB after the computation
for ExitMBB
has converged. With this change, all basic blocks have a recomputation
of the live-ins
for each loop iteration. This can result in less or more calls,
depending on the
situation.
@redstar redstar deleted the knacke/refactorliveregs branch April 16, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants