[ModuloSchedule] Make PeelingModuloScheduleExpander inheritable.

[hgreving]

Summary

Basically a NFC, but allows subclasses access to the entire PeelingModuloScheduleExpander
class. We are doing this to allow backends, particularly ones that are not necessarily
upstreamed, to inherit from PeelingModuloScheduleExpander and access its basic structures.

Renames Info into LoopInfo for consistency in PeelingModuloScheduleExpander.

[jmolloy]

LGTM!

[jmolloy]

(assuming the LIT failures are fixed of course!)

[jfb]

llvm/include/llvm/CodeGen/ModuloSchedule.h

286

This generates the following diagnostic when building:

llvm/include/llvm/CodeGen/ModuloSchedule.h:279:7: warning: 'llvm::PeelingModuloScheduleExpander' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class PeelingModuloScheduleExpander {
      ^

If you ever destroy a derived class while holding a PeelingModuloScheduleExpander* you'll have a bad time.

Fixed here:
https://github.com/llvm/llvm-project/commit/c7586444ca787c3845ac4ad0bd603709f2abbb0f

[dblaikie]

Adding an extension point with no testing inside LLVM seems a bit problematic - is there no existing target that could use this? Or a reasonable scale of unit test that could exercise this extension point without a full-blown target?

(also this produced a warning about the fact that this type doesn't have a virtual dtor - was fixed by @jfb in c7586444ca787c3845ac4ad0bd603709f2abbb0f )

[hgreving]

In D82673#2125936, @dblaikie wrote:

Adding an extension point with no testing inside LLVM seems a bit problematic - is there no existing target that could use this? Or a reasonable scale of unit test that could exercise this extension point without a full-blown target?

(also this produced a warning about the fact that this type doesn't have a virtual dtor - was fixed by @jfb in c7586444ca787c3845ac4ad0bd603709f2abbb0f )

The change is basically NFC. It was made in order to allow us using the upstream pass by inheriting from it. I'm not sure what test you would write, are you thinking about a test that tests protected vs. private? Heaxgon is the only upstream target upstream using the modulo scheduler. Full disclosure, we're a bit short staffed and are not in a position writing Hexagon tests. We _would_ like to use the upstream pass and when the time comes, add our improvements to LLVM upstream. Making the peeling modulo scheduler inheritable is a reasonable interface change, because different subtargets have different peeling requirements while sharing common semantics. The change simply allowed for the entire interface to be inheritable. Reality is, if the change is unwanted, we would end up downstreaming the pass entirely and developement on the upstream pass would stop - for now. If you're ok with the change, I think it's beneficial to everybody at this point. LMK, thanks for considering

[dblaikie]

In D82673#2126023, @hgreving wrote:

In D82673#2125936, @dblaikie wrote:

Adding an extension point with no testing inside LLVM seems a bit problematic - is there no existing target that could use this? Or a reasonable scale of unit test that could exercise this extension point without a full-blown target?

(also this produced a warning about the fact that this type doesn't have a virtual dtor - was fixed by @jfb in c7586444ca787c3845ac4ad0bd603709f2abbb0f )

The change is basically NFC. It was made in order to allow us using the upstream pass by inheriting from it.

Why does "expand" need to be virtual to inherit from it? The current caller in-tree doesn't call this function in a setting that would be applicable to polymorphism, for instance - it creates a concrete PeelingModuloScheduleExpander instance and calls "expand" directly on that.

I'm not sure what test you would write, are you thinking about a test that tests protected vs. private?

Nah - I was assuming the in-tree use was structured in a way that, while there might be only one implementation now, there might be some separation between construction and usage - such that maybe that code (that had usage separate from construction) could be unit tested with, say, a stub derived class to show the overriding behavior being exercised. But since usage is not separate from construction that doesn't seem to be possible/reasonable.

If out-of-tree uses would look much like the in-tree use, it doesn't seem to me like virtuality is required - the derived class would define its own expand function (& it could derive privately from the base class to hide the original implementation) and users would call that one.
If out-of-tree code wants to use dynamic polymorphism here, it seems like it could do so by introducing a small class deriving privately and exposing a virtual function.

But without any use in-tree that looks at least /roughly/ like the sort of thing that would benefit from dynamic polymorphism - I think that's best left out of tree.

Hexagon is the only upstream target upstream using the modulo scheduler. Full disclosure, we're a bit short staffed and are not in a position writing Hexagon tests. We _would_ like to use the upstream pass and when the time comes, add our improvements to LLVM upstream. Making the peeling modulo scheduler inheritable is a reasonable interface change, because different subtargets have different peeling requirements while sharing common semantics.

The change simply allowed for the entire interface to be inheritable. Reality is, if the change is unwanted, we would end up downstreaming the pass entirely and developement on the upstream pass would stop - for now. If you're ok with the change, I think it's beneficial to everybody at this point. LMK, thanks for considering

Who else has a vested interest in having this in-tree? Perhaps they could be CC'd in this review to discuss this.

& would a small virtual shim out-of-tree allow you to work downstream while still having this pass in-use upstream?

[hgreving]

In short - the virtuality is not the issue, the inheritability is. The class can be non-virtual, no problem for us. I would like to reuse existing code in the upstream pass that is currently "private". Hence, the "protected" part is important. It also makes sense generally, because a software pipeliner on a specific subtarget may follow different expansion strategies. If you would like to revert the virtuality, no problem at all, if you keep the protected inheritance. I think we should design a better defined interface for this class in the long run. Only one target upstream and us downstream are using it AFAIK, but as we are supporting more targets, we may come up with something better. SG? Can pull in Hexagon people, yes

[dblaikie]

In D82673#2129786, @hgreving wrote:

In short - the virtuality is not the issue, the inheritability is. The class can be non-virtual, no problem for us. I would like to reuse existing code in the upstream pass that is currently "private". Hence, the "protected" part is important. It also makes sense generally, because a software pipeliner on a specific subtarget may follow different expansion strategies. If you would like to revert the virtuality, no problem at all, if you keep the protected inheritance. I think we should design a better defined interface for this class in the long run. Only one target upstream and us downstream are using it AFAIK, but as we are supporting more targets, we may come up with something better. SG? Can pull in Hexagon people, yes

Ah, fair enough. Thanks for explaining!

I've removed the virtual dtor and virtuality from expand in 7a99aab8692c58558b62e9a66120886b8a70fab8

[hgreving]

In D82673#2134669, @dblaikie wrote:

In D82673#2129786, @hgreving wrote:

In short - the virtuality is not the issue, the inheritability is. The class can be non-virtual, no problem for us. I would like to reuse existing code in the upstream pass that is currently "private". Hence, the "protected" part is important. It also makes sense generally, because a software pipeliner on a specific subtarget may follow different expansion strategies. If you would like to revert the virtuality, no problem at all, if you keep the protected inheritance. I think we should design a better defined interface for this class in the long run. Only one target upstream and us downstream are using it AFAIK, but as we are supporting more targets, we may come up with something better. SG? Can pull in Hexagon people, yes

Ah, fair enough. Thanks for explaining!

I've removed the virtual dtor and virtuality from expand in 7a99aab8692c58558b62e9a66120886b8a70fab8

Ok. Thanks

llvm/include/llvm/CodeGen/ModuloSchedule.h

public:
   /// Returns the newly rewritten kernel block, or nullptr if this was
   /// optimized away.
   MachineBasicBlock *getRewrittenKernel() { return NewKernel; }
 };
 
 /// A reimplementation of ModuloScheduleExpander. It works by generating a
 /// standalone kernel loop and peeling out the prologs and epilogs.
 class PeelingModuloScheduleExpander {
+public:
+  PeelingModuloScheduleExpander(MachineFunction &MF, ModuloSchedule &S,
+                                LiveIntervals *LIS)
+      : Schedule(S), MF(MF), ST(MF.getSubtarget()), MRI(MF.getRegInfo()),
+        TII(ST.getInstrInfo()), LIS(LIS) {}
+
+  virtual void expand();
+
+  /// Runs ModuloScheduleExpander and treats it as a golden input to validate
+  /// aspects of the code generated by PeelingModuloScheduleExpander.
+  void validateAgainstModuloScheduleExpander();
+
+protected:
   ModuloSchedule &Schedule;
   MachineFunction &MF;
   const TargetSubtargetInfo &ST;
   MachineRegisterInfo &MRI;
   const TargetInstrInfo *TII;
   LiveIntervals *LIS;
 
   /// The original loop block that gets rewritten in-place.
protected:
   DenseMap<std::pair<MachineBasicBlock *, MachineInstr *>, MachineInstr *>
       BlockMIs;
 
   /// State passed from peelKernel to peelPrologAndEpilogs().
   std::deque<MachineBasicBlock *> PeeledFront, PeeledBack;
   /// Illegal phis that need to be deleted once we re-link stages.
   SmallVector<MachineInstr *, 4> IllegalPhisToDelete;
 
-public:
-  PeelingModuloScheduleExpander(MachineFunction &MF, ModuloSchedule &S,
-                                LiveIntervals *LIS)
-      : Schedule(S), MF(MF), ST(MF.getSubtarget()), MRI(MF.getRegInfo()),
-        TII(ST.getInstrInfo()), LIS(LIS) {}
-
-  void expand();
-
-  /// Runs ModuloScheduleExpander and treats it as a golden input to validate
-  /// aspects of the code generated by PeelingModuloScheduleExpander.
-  void validateAgainstModuloScheduleExpander();
-
-protected:
   /// Converts BB from the original loop body to the rewritten, pipelined
   /// steady-state.
   void rewriteKernel();
 
-private:
   /// Peels one iteration of the rewritten kernel (BB) in the specified
   /// direction.
   MachineBasicBlock *peelKernel(LoopPeelDirection LPD);
   // Delete instructions whose stage is less than MinStage in the given basic
   // block.
   void filterInstructions(MachineBasicBlock *MB, int MinStage);
   // Move instructions of the given stage from sourceBB to DestBB. Remap the phi
   // instructions to keep a valid IR.
unsigned getStage(MachineInstr *MI) {
     if (CanonicalMIs.count(MI))
       MI = CanonicalMIs[MI];
     return Schedule.getStage(MI);
   }
   /// Helper function to find the right canonical register for a phi instruction
   /// coming from a peeled out prologue.
   Register getPhiCanonicalReg(MachineInstr* CanonicalPhi, MachineInstr* Phi);
   /// Target loop info before kernel peeling.
-  std::unique_ptr<TargetInstrInfo::PipelinerLoopInfo> Info;
+  std::unique_ptr<TargetInstrInfo::PipelinerLoopInfo> LoopInfo;
 };
 
 /// Expander that simply annotates each scheduled instruction with a post-instr
 /// symbol that can be consumed by the ModuloScheduleTest pass.
 ///
 /// The post-instr symbol is a way of annotating an instruction that can be
 /// roundtripped in MIR. The syntax is:
 ///   MYINST %0, post-instr-symbol <mcsymbol Stage-1_Cycle-5>

llvm/lib/CodeGen/ModuloSchedule.cpp

void PeelingModuloScheduleExpander::fixupBranches() {
   for (auto PI = Prologs.rbegin(), EI = Epilogs.rbegin(); PI != Prologs.rend();
        ++PI, ++EI, --TC) {
     MachineBasicBlock *Prolog = *PI;
     MachineBasicBlock *Fallthrough = *Prolog->succ_begin();
     MachineBasicBlock *Epilog = *EI;
     SmallVector<MachineOperand, 4> Cond;
     TII->removeBranch(*Prolog);
     Optional<bool> StaticallyGreater =
-        Info->createTripCountGreaterCondition(TC, *Prolog, Cond);
+        LoopInfo->createTripCountGreaterCondition(TC, *Prolog, Cond);
     if (!StaticallyGreater.hasValue()) {
       LLVM_DEBUG(dbgs() << "Dynamic: TC > " << TC << "\n");
       // Dynamically branch based on Cond.
       TII->insertBranch(*Prolog, Epilog, Fallthrough, Cond, DebugLoc());
     } else if (*StaticallyGreater == false) {
       LLVM_DEBUG(dbgs() << "Static-false: TC > " << TC << "\n");
       // Prolog never falls through; branch to epilog and orphan interior
       // blocks. Leave it to unreachable-block-elim to clean up.
if (!StaticallyGreater.hasValue()) {
       for (MachineInstr &P : Epilog->phis()) {
         P.RemoveOperand(4);
         P.RemoveOperand(3);
       }
     }
   }
 
   if (!KernelDisposed) {
-    Info->adjustTripCount(-(Schedule.getNumStages() - 1));
-    Info->setPreheader(Prologs.back());
+    LoopInfo->adjustTripCount(-(Schedule.getNumStages() - 1));
+    LoopInfo->setPreheader(Prologs.back());
   } else {
-    Info->disposed();
+    LoopInfo->disposed();
   }
 }
 
 void PeelingModuloScheduleExpander::rewriteKernel() {
   KernelRewriter KR(*Schedule.getLoop(), Schedule);
   KR.rewrite();
 }
 
 void PeelingModuloScheduleExpander::expand() {
   BB = Schedule.getLoop()->getTopBlock();
   Preheader = Schedule.getLoop()->getLoopPreheader();
   LLVM_DEBUG(Schedule.dump());
-  Info = TII->analyzeLoopForPipelining(BB);
-  assert(Info);
+  LoopInfo = TII->analyzeLoopForPipelining(BB);
+  assert(LoopInfo);
 
   rewriteKernel();
   peelPrologAndEpilogs();
   fixupBranches();
 }
 
 void PeelingModuloScheduleExpander::validateAgainstModuloScheduleExpander() {
   BB = Schedule.getLoop()->getTopBlock();