[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())
----------------
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...


================
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.
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995