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

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:70
+template <typename ContextT>
+auto GenericCycle<ContextT>::getCyclePreheader() const -> BlockT * {
+  if (!isReducible())
----------------
MatzeB wrote:
> sameerds wrote:
> > MatzeB wrote:
> > > I did not even know this is legal C++ syntax; Anyway please be consistent with the rest of LLVM here!
> > There is no other precedent in LLVM for this to be consistent with. The proposed change of putting BlockT* as the return type will not work because BlockT is defined inside the GenericCycle<ContextT> class and cannot be used until that class is named. The alternative is to say "GenericCycle<ContextT>::BlockT", but I am very strongly opposed to that kind of repetition.
> I would rather see the longer `GenericCycle<ContextT>::BlockT` than the unusual trailing return syntax that isn't even necessary here.
> 
> Seems like we have to agree to disagree here. And I guess that means leaving it unchanged if I cannot convince you...
I leave it as now to make sure it is uniform at least in this file. We can change all these functions with a follow-up NFC patch if you insist.


================
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;
----------------
MatzeB wrote:
> 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`?
This is from `getLoopPreheader()` and `isLegalToHoistInto()` check was intentionally added in https://reviews.llvm.org/D34487. And IMO, we also need this for cycle preheader?


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:103
 
+  auto getHeaders() const -> const SmallVector<BlockT *, 1> & {
+    return Entries;
----------------
MatzeB wrote:
> 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 
Thanks. I will commit another patch to rename above `getHeader()` later.


================
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.
----------------
MatzeB wrote:
> sameerds wrote:
> > MatzeB wrote:
> > > 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`)
> > The document does define preheader, under the section Terminology:
> > 
> > https://llvm.org/docs/LoopTerminology.html#terminology
> > 
> > Good catch about isLegalToHoistInto. But maybe only that check needs to be moved out of this file? The preheader itself is still defined purely in terms of CFG structure.
> * Oh, I was reading `CycleTerminology` and missed this one. I would still recommend to add 1 or 2 sentences here to what a preheader is and maybe also add it to `CycleTerminology.rst`.
> 
> * Moving just the `isLegalToHoistInto` call works too of course.
I will handle `isLegalToHoistInto` when we decide where should we put it.


================
Comment at: llvm/include/llvm/ADT/GenericCycleInfo.h:219
   }
+  //@}
 };
----------------
MatzeB wrote:
> Consider just pushing this as a small fix directly to git without review.
Committed https://reviews.llvm.org/rG0ca2b93cc2864ad33937f21351f0d34dd5819aa2


================
Comment at: llvm/include/llvm/CodeGen/MachineCycleAnalysis.h:45
+
+  // TODO: verify analysis
+};
----------------
MatzeB wrote:
> Do we need a `TODO` comment here? Will the verify analysis added at this particular line?
Moved from `lib/CodeGen/MachineCycleAnalysis.cpp`. I thought, it is a placeholder for a declare of verify function?


================
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);
----------------
MatzeB wrote:
> Do we need a `TODO` comment?
This function should be implemented for both MIR and IR. Now for machinesink, I just implement MIR version, once IR version is implemented, we can move this function to the template class `GenericCycle`. So I add this TODO, do you mean we don't need TODO for this case?


================
Comment at: llvm/lib/CodeGen/MachineCycleAnalysis.cpp:124-125
+
+    if (!MO.isUse())
+      continue;
+
----------------
MatzeB wrote:
> 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.
OK.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995