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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

Thanks, I think this will be good to go when all the nitpicks are addressed!



================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:71-72
+auto GenericCycle<ContextT>::getCyclePreheader() const -> BlockT * {
+  if (!isReducible())
+    return nullptr;
+
----------------
This is already checked in `getCyclePredecessor`


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:82
+  // Make sure we are allowed to hoist instructions into the predecessor.
+  if (!Predecessor->isLegalToHoistInto())
+    return nullptr;
----------------
Everything in the `Cycle` info appears to be only concerned about the structure of the CFG. This is the only function that looks at non-structural attributes like if something is an exception landing pad. I feel like it's probably better to have this helper function somewhere else and not be a member of `GenericCycle`.

How about keeping it local to `MachineSink.cpp`?


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:97
+  BlockT *Header = getHeader();
+  for (const auto Pred : predecessors(Header)) {
+    if (!contains(Pred)) {
----------------



================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:103
 
+  auto getHeaders() const -> const SmallVector<BlockT *, 1> & {
+    return Entries;
----------------
MatzeB wrote:
> as above
- Other functions are called `isEntry` and `printEntries` and the CycleTerminology documentation calls them entries as well. So this should be called `getEntries()` I think.

- Use `SmallVectorImpl<T>` without specifying the size for APIs 


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:131-132
 
+  /// Return the preheader block for this cycle. Pre-header is well-defined for
+  /// reducible cycle as listed in docs/LoopTerminology.rst. Return null for
+  /// irreducible cycles.
----------------
As far as I can tell `LoopTerminology.rst` defines the term *Header* but not pre-header. Please put a definition into the comment here.

(And consider moving the function out of here anyway as above, since it checks for non-structural properties in `isLegalToHoistInto`)


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:136-137
+
+  /// If the cycle has exactly one unique predecessor outside of the loop,
+  /// return it, otherwise return null. Return null for irreducible cycles.
+  BlockT *getCyclePredecessor() const;
----------------
I think we should talk about "cycles" not "loops" here. How about this:


================
Comment at: llvm/lib/CodeGen/MachineCycleAnalysis.cpp:17-18
 
 template class llvm::GenericCycleInfo<llvm::MachineSSAContext>;
 template class llvm::GenericCycle<llvm::MachineSSAContext>;
 
----------------
I assume this can be removed given that you dropped the `extern` declarations in the header?


================
Comment at: llvm/lib/CodeGen/MachineCycleAnalysis.cpp:124-125
+
+    if (!MO.isUse())
+      continue;
+
----------------
FWIW: We should probably change this to `!MO.readsReg()` (because moving `undef` use operands around should be no problem); but it's probably out of the scope of this diff and we better do that separately.


================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:237-239
+    void FindLoopSinkCandidates(MachineCycle *L, MachineBasicBlock *BB,
                                 SmallVectorImpl<MachineInstr *> &Candidates);
+    bool SinkIntoLoop(MachineCycle *L, MachineInstr &I);
----------------



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:470-472
+    SmallVector<MachineCycle *, 8> Loops(CI->toplevel_begin(),
+                                         CI->toplevel_end());
     for (auto *L : Loops) {
----------------



================
Comment at: llvm/lib/CodeGen/MachineSink.cpp:646-648
+  if (CI->getCycle(FromBB) == CI->getCycle(ToBB) && CI->getCycle(FromBB) &&
+      (!CI->getCycle(FromBB)->isReducible() ||
+       CI->getCycle(ToBB)->getHeader() == ToBB))
----------------
Move the `getCycle` results into variables so we don't need to lookup the same block multiple times in the hashmap?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995