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

Mon May 23 20:26:12 PDT 2022

sameerds accepted this revision.
sameerds added a comment.
This revision is now accepted and ready to land.

LGTM! My sincere apologies for the delay ... I have no excuse :(

================
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.
FWIW, this got discussed in the original review where this file was added:

https://reviews.llvm.org/D112696#3175521

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995