[PATCH] D113150: Lift VLIWResourceModel, VLIWMachineScheduler, and ConvergingVLIWScheduler into CodeGen/VLIWMachineScheduler

[James Nagurne via Phabricator via llvm-commits]

JamesNagurne added a comment.

Comments for review added.

This has passed on my downstream, all-target Release build with expensive checks and assertions. The upstream base commit is 870fc844 <https://reviews.llvm.org/rG870fc844d11e3e7b75bf62858953e08df1b1d94c>:"[ORC-RT] Add SPS serialization for span<const char> / SPSSequence<char>."



================
Comment at: llvm/include/llvm/CodeGen/VLIWMachineScheduler.h:34
+protected:
+  const TargetInstrInfo *TII;
+
----------------
Added a TargetInstrInfo so that targets have access to it in overrides


================
Comment at: llvm/include/llvm/CodeGen/VLIWMachineScheduler.h:73
+
+  virtual bool hasDependence(const SUnit *SUd, const SUnit *SUu);
+  virtual bool isResourceAvailable(SUnit *SU, bool IsTop);
----------------
API overrides:


  - Check if two SUnits conflict
  - Check if SUnit can be added to current state
  - Actually add item to current state








================
Comment at: llvm/include/llvm/CodeGen/VLIWMachineScheduler.h:77
+  unsigned getTotalPackets() const { return TotalPackets; }
+  size_t getPacketInstCount() const { return Packet.size(); }
+  bool isInPacket(SUnit *SU) const { return is_contained(Packet, SU); }
----------------
This API is not currently called upstream, but is important for downstream, so I've added it.


================
Comment at: llvm/include/llvm/CodeGen/VLIWMachineScheduler.h:81
+protected:
+  virtual DFAPacketizer *createPacketizer(const TargetSubtargetInfo &STI)
+      const {
----------------
This function is redundant with the current design. However, downstream we've made changes to allow the concept of a "Packetizer", which has the API of DFAPacketizer, but without the requirement of being backed by the DFA created by tablegen. We then implement our own Packetizers, of which one is returned by CreateTargetScheduleState. Others may exist, however, and necessitate the ability to swap in custom implementations here.

I'm not opposed to removing it for clarity's sake.


================
Comment at: llvm/include/llvm/CodeGen/VLIWMachineScheduler.h:131
+  // heuristic components for cost computation.
+  static constexpr unsigned PriorityOne = 200;
+  static constexpr unsigned PriorityTwo = 50;
----------------
Constants moved here to be static members instead of static locals in HexagonMachineScheduler.cpp

I've noticed that static constexpr members, when utilized in LLVM, do not have backing definitions in source files. This is actually required by the C++ standard in case an expression like '&PriorityTwo' occurs.

The later addition of consteval in C++20 removes the definition requirement, but since it seems commonplace to elide the definition in LLVM, I left it as-is.


================
Comment at: llvm/include/llvm/CodeGen/VLIWMachineScheduler.h:260
+protected:
+  virtual VLIWResourceModel *createVLIWResourceModel(
+      const TargetSubtargetInfo &STI, const TargetSchedModel *SchedModel) const;
----------------
Helper to allow targets to inject their own VLIWResourceModel implementation


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:96
 #ifndef NDEBUG
-static cl::opt<bool> ViewMISchedDAGs("view-misched-dags", cl::Hidden,
+cl::opt<bool> ViewMISchedDAGs("view-misched-dags", cl::Hidden,
   cl::desc("Pop up a window to show MISched dags after they are processed"));
----------------
It's very helpful to view dags, but the option to do this is locked behind a static variable.
I've seen elsewhere that using a non-static cl::opt and an extern declaration elsewhere is a valid way to make the option available in other source, and applied that here.


================
Comment at: llvm/lib/CodeGen/VLIWMachineScheduler.cpp:75
+    return false;
+
+  for (const auto &S : SUd->Succs) {
----------------
There is Hexagon-specific code here that has been lifted


================
Comment at: llvm/lib/CodeGen/VLIWMachineScheduler.cpp:263
+  delete Bot.ResourceModel;
+  Top.ResourceModel = createVLIWResourceModel(STI, DAG->getSchedModel());
+  Bot.ResourceModel = createVLIWResourceModel(STI, DAG->getSchedModel());
----------------
Uses of the VLIWResourceModel factory function


================
Comment at: llvm/lib/CodeGen/VLIWMachineScheduler.cpp:676
+  }
+
+  // Give preference to a zero latency instruction if the dependent
----------------
There is Hexagon-specific code here that has been lifted


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113150