[PATCH] D123995: [MachineSink] replace MachineLoop with MachineCycle

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:70
+template <typename ContextT>
+auto GenericCycle<ContextT>::getCyclePreheader() const -> BlockT * {
+  if (!isReducible())
----------------
I did not even know this is legal C++ syntax; Anyway please be consistent with the rest of LLVM here!


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:89
+template <typename ContextT>
+auto GenericCycle<ContextT>::getCyclePredecessor() const -> BlockT * {
+  if (!isReducible())
----------------



================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:103
 
+  auto getHeaders() const -> const SmallVector<BlockT *, 1> & {
+    return Entries;
----------------
as above


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:219
   }
+  //@}
 };
----------------
Consider just pushing this as a small fix directly to git without review.


================
Comment at: llvm/include/llvm/CodeGen/MachineCycleAnalysis.h:45
+
+  // TODO: verify analysis
+};
----------------
Do we need a `TODO` comment here? Will the verify analysis added at this particular line?


================
Comment at: llvm/include/llvm/CodeGen/MachineCycleAnalysis.h:58-59
+
+// TODO: add this function to GenericCycle template after implementing IR
+//       version.
+bool isCycleInvariant(const MachineCycle *Cycle, MachineInstr &I);
----------------
Do we need a `TODO` comment?


================
Comment at: llvm/include/llvm/CodeGen/MachineSSAContext.h:31-32
 inline auto predecessors(MachineBasicBlock *BB) { return BB->predecessors(); }
+inline auto succ_size(MachineBasicBlock *BB) { return BB->succ_size(); }
+inline auto pred_size(MachineBasicBlock *BB) { return BB->pred_size(); }
 
----------------
Explicit types help everyone reading the code.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:384
+void MachineSinking::FindLoopSinkCandidates(
+    MachineCycle *L, MachineBasicBlock *BB,
     SmallVectorImpl<MachineInstr *> &Candidates) {
----------------
Rename `L` to `C` (or better `Cycle`)?


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:472-473
+                                         CI->toplevel_end());
     for (auto *L : Loops) {
-      MachineBasicBlock *Preheader = LI->findLoopPreheader(L);
+      MachineBasicBlock *Preheader = L->getCyclePreheader();
       if (!Preheader) {
----------------
Rename L/Loop to Cycle/Cycles ?

There's more place below that where the same applies.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123995/new/

https://reviews.llvm.org/D123995