[DFAPacketizer] Move DefaultVLIWScheduler class declaration to header file

[DarshanRamakant]

Summary

This change moves "DefaultVLIWScheduler" class declaration from
DFAPacketizer.cpp to DFAPacketizer.h.
This is needed because there is a protected class member of
type "DefaultVLIWScheduler*" in "VLIWPacketizerList" class.
The derived classes cannot use this memeber unless declaration
is available to it. More specifically :

// Without this change

class HexagonPacketizerList : public VLIWPacketizerList {
  public :
	HexagonPacketizerList() {
	// Below line will cause incomplete class error since
	// declaration was not available through header.
	VLIWScheduler->schedule();
  }
}

[dexonsmith]

I don't know much about this code; resigning as reviewer. But I took a quick look. I wonder if this the right fix, or whether VLIWPacketizerList::VLIWScheduler's type should be ScheduleDAGInstrs, and downcast when necessary on use, allowing DefaultVLIWScheduler to stay private. (Up to others to sort out!)

[DarshanRamakant]

In D139767#3986476, @dexonsmith wrote:

I don't know much about this code; resigning as reviewer. But I took a quick look. I wonder if this the right fix, or whether VLIWPacketizerList::VLIWScheduler's type should be ScheduleDAGInstrs, and downcast when necessary on use, allowing DefaultVLIWScheduler to stay private. (Up to others to sort out!)

Thanks @dexonsmith for the suggestion. But I think even if we want to downcast ( dynamic_cast) the declaration should be available to the derived class, which is not the case in the current implementation.

[dexonsmith]

In D139767#3986728, @DarshanRamakant wrote:

In D139767#3986476, @dexonsmith wrote:

I don't know much about this code; resigning as reviewer. But I took a quick look. I wonder if this the right fix, or whether VLIWPacketizerList::VLIWScheduler's type should be ScheduleDAGInstrs, and downcast when necessary on use, allowing DefaultVLIWScheduler to stay private. (Up to others to sort out!)

Thanks @dexonsmith for the suggestion. But I think even if we want to downcast ( dynamic_cast) the declaration should be available to the derived class, which is not the case in the current implementation.

Not sure I'm following, maybe I've misread something, but it seems like:

• Your example (HexagonPacketizerList) could just call ScheduleDAGInstrs::schedule(), which is a public method. No need to downcast.
• Within llvm/lib/CodeGen/DFAPacketizer.cpp, code that needs to call addMutation() might need to downcast. This would be a static_cast though, since the implementation knows the type. (Note: LLVM builds without RTTI, so dynamic_cast never works. If you don't know what type to downcast to, you need to save type info explicitly somehow in the base class.)

[DarshanRamakant]

In D139767#3986749, @dexonsmith wrote:

In D139767#3986728, @DarshanRamakant wrote:

In D139767#3986476, @dexonsmith wrote:

I don't know much about this code; resigning as reviewer. But I took a quick look. I wonder if this the right fix, or whether VLIWPacketizerList::VLIWScheduler's type should be ScheduleDAGInstrs, and downcast when necessary on use, allowing DefaultVLIWScheduler to stay private. (Up to others to sort out!)

Thanks @dexonsmith for the suggestion. But I think even if we want to downcast ( dynamic_cast) the declaration should be available to the derived class, which is not the case in the current implementation.

Not sure I'm following, maybe I've misread something, but it seems like:

• Your example (HexagonPacketizerList) could just call ScheduleDAGInstrs::schedule(), which is a public method. No need to downcast.
• Within llvm/lib/CodeGen/DFAPacketizer.cpp, code that needs to call addMutation() might need to downcast. This would be a static_cast though, since the implementation knows the type. (Note: LLVM builds without RTTI, so dynamic_cast never works. If you don't know what type to downcast to, you need to save type info explicitly somehow in the base class.)

My idea was to make "DefaultVLIWScheduler" class type available to derived classes of "VLIWPacketizerList" so that even "addMutatation()" can be used in derived classes. But I also ok with changing the member type to base class. Waiting for other reviewers comments.

[DarshanRamakant]

Gentle reminder to review the change !

[DarshanRamakant]

Hi All,

I have been waiting for a review for this change for 3 weeks. Please review
the code and allow me to merge the changes if the code looks good.

Thanks,
Darshan Bhat

[kparzysz]

The DefaultVLIWScheduler class is really a thin wrapper on top of ScheduleDAGInstrs that holds/applies graph mutations. It doesn't really do anything useful on its own, and there isn't much there that you can customize. I'm not sure why you'd want to make it available, could you explain your use case?

[DarshanRamakant]

llvm/include/llvm/CodeGen/DFAPacketizer.h

143

@kparzysz , this type will become incomplete class type for the child class of VLIWPacketizerList unless declaration of "DefaultVLIWScheduler" is moved to this header.
Since this member is a protected member, I assume the design intention was to make it available to child class.

My usecase was to access this member and call "schedule()" method of it.

This patch just moves the declaration from cpp file to header file.

[DarshanRamakant]

Please let me know how to submit this change if there are no other comments / concerns.

[kparzysz]

I'm not sure how it helps, but this isn't a huge deal anyways.

[DarshanRamakant]

In D139767#4052216, @kparzysz wrote:

I'm not sure how it helps, but this isn't a huge deal anyways.

Thanks @kparzysz. I dont have push rights (not sure how to get it) , can you please push this change on behalf of me ?

llvm/include/llvm/CodeGen/DFAPacketizer.h

 // indicate that the instruction cannot be added to the current packet.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_DFAPACKETIZER_H
 #define LLVM_CODEGEN_DFAPACKETIZER_H
 
 #include "llvm/CodeGen/MachineBasicBlock.h"
+#include "llvm/CodeGen/ScheduleDAGInstrs.h"
+#include "llvm/CodeGen/ScheduleDAGMutation.h"
 #include "llvm/Support/Automaton.h"
 #include <cstdint>
 #include <map>
 #include <memory>
 #include <utility>
 #include <vector>
 
 namespace llvm {
 
-class DefaultVLIWScheduler;
 class ScheduleDAGMutation;
 class InstrItineraryData;
 class MachineFunction;
 class MachineInstr;
 class MachineLoopInfo;
 class MCInstrDesc;
 class SUnit;
 class TargetInstrInfo;
 
+// This class extends ScheduleDAGInstrs and overrides the schedule method
+// to build the dependence graph.
+class DefaultVLIWScheduler : public ScheduleDAGInstrs {
+private:
+  AAResults *AA;
+  /// Ordered list of DAG postprocessing steps.
+  std::vector<std::unique_ptr<ScheduleDAGMutation>> Mutations;
+
+public:
+  DefaultVLIWScheduler(MachineFunction &MF, MachineLoopInfo &MLI,
+                       AAResults *AA);
+
+  // Actual scheduling work.
+  void schedule() override;
+
+  /// DefaultVLIWScheduler takes ownership of the Mutation object.
+  void addMutation(std::unique_ptr<ScheduleDAGMutation> Mutation) {
+    Mutations.push_back(std::move(Mutation));
+  }
+
+protected:
+  void postprocessDAG();
+};
+
 class DFAPacketizer {
 private:
   const InstrItineraryData *InstrItins;
   Automaton<uint64_t> A;
   /// For every itinerary, an "action" to apply to the automaton. This removes
   /// the redundancy in actions between itinerary classes.
   ArrayRef<unsigned> ItinActions;
 

 // found, a target API call is made to prune the dependence.
 class VLIWPacketizerList {
 protected:
   MachineFunction &MF;
   const TargetInstrInfo *TII;
   AAResults *AA;
 
   // The VLIW Scheduler.
   DefaultVLIWScheduler *VLIWScheduler;
   // Vector of instructions assigned to the current packet.
   std::vector<MachineInstr*> CurrentPacketMIs;
   // DFA resource tracker.
   DFAPacketizer *ResourceTracker;
   // Map: MI -> SU.
   std::map<MachineInstr*, SUnit*> MIToSUnit;
 
 public:

llvm/lib/CodeGen/DFAPacketizer.cpp

 
 #include "llvm/CodeGen/DFAPacketizer.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBundle.h"
 #include "llvm/CodeGen/ScheduleDAG.h"
-#include "llvm/CodeGen/ScheduleDAGInstrs.h"
-#include "llvm/CodeGen/ScheduleDAGMutation.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetSubtargetInfo.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
unsigned DFAPacketizer::getUsedResources(unsigned InstIdx) {
   // instruction. The 0th instruction is the base case.
   if (InstIdx == 0)
     return RS[0];
   // Return the difference between the cumulative resources used by InstIdx and
   // its predecessor.
   return RS[InstIdx] ^ RS[InstIdx - 1];
 }
 
-namespace llvm {
-
-// This class extends ScheduleDAGInstrs and overrides the schedule method
-// to build the dependence graph.
-class DefaultVLIWScheduler : public ScheduleDAGInstrs {
-private:
-  AAResults *AA;
-  /// Ordered list of DAG postprocessing steps.
-  std::vector<std::unique_ptr<ScheduleDAGMutation>> Mutations;
-
-public:
-  DefaultVLIWScheduler(MachineFunction &MF, MachineLoopInfo &MLI,
-                       AAResults *AA);
-
-  // Actual scheduling work.
-  void schedule() override;
-
-  /// DefaultVLIWScheduler takes ownership of the Mutation object.
-  void addMutation(std::unique_ptr<ScheduleDAGMutation> Mutation) {
-    Mutations.push_back(std::move(Mutation));
-  }
-
-protected:
-  void postprocessDAG();
-};
-
-} // end namespace llvm
-
 DefaultVLIWScheduler::DefaultVLIWScheduler(MachineFunction &MF,
                                            MachineLoopInfo &MLI,
                                            AAResults *AA)
     : ScheduleDAGInstrs(MF, &MLI), AA(AA) {
   CanHandleTerminators = true;
 }
 
 /// Apply each ScheduleDAGMutation step in order.