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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds requested changes to this revision.
sameerds added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/ADT/GenericCycleImpl.h:70
+template <typename ContextT>
+auto GenericCycle<ContextT>::getCyclePreheader() const -> BlockT * {
+  if (!isReducible())
----------------
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.


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


================
Comment at: llvm/include/llvm/CodeGen/MachineCycleAnalysis.h:22-23
 
-extern template class GenericCycleInfo<MachineSSAContext>;
-extern template class GenericCycle<MachineSSAContext>;
-
----------------
Why was this removed? This is necessary to prevent automatic instantiation of the template at the point of use. This particular template instance is defined once in MachineCycleAnalysis.cpp


================
Comment at: llvm/lib/CodeGen/MachineCycleAnalysis.cpp:9-13
 #include "llvm/ADT/GenericCycleImpl.h"
-#include "llvm/CodeGen/MachineFunctionPass.h"
-#include "llvm/CodeGen/MachineSSAContext.h"
-#include "llvm/InitializePasses.h"
+#include "llvm/CodeGen/MachineCycleAnalysis.h"
+#include "llvm/CodeGen/MachineRegisterInfo.h"
+#include "llvm/CodeGen/TargetInstrInfo.h"
+#include "llvm/CodeGen/TargetSubtargetInfo.h"
----------------
This ordering of headers is wrong. MachineCycleAnalysis.h should be the first header. I am surprised that clang-format did not do this automatically.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123995