[PATCH] D51160: Adjust MIScheduler to use ProcResource counts

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Do you have any benchmark data you could share with this patch? I am curious what the impact is and if there are any  regressions.



================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:686
+  // it.
+  SmallVector<unsigned, 16> ProcessorResourceList;
+
----------------
This name is too generic I think. this is only relevant to ReservedCycles. Also it is basically a map, right?


================
Comment at: llvm/include/llvm/CodeGen/TargetSchedule.h:119
+  /// Get the processor resource table.
+  const MCProcResourceDesc *getProcResourceTable() const {
+    return SchedModel.ProcResourceTable;
----------------
Is there a benefit if adding this over just using getProcResource? Leaking the table may make future changes harder.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:1974
+std::pair<unsigned, unsigned> SchedBoundary::
+getNextResourceCycle(unsigned PIdx, unsigned ResCount, unsigned Cycles) {
+  unsigned MinNextUnreserved = InvalidCycle;
----------------
ResCount not used here? I think you can remove it, as you get it from the model in the function.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:2036
       unsigned Cycles = PE.Cycles;
-      unsigned NRCycle = getNextResourceCycle(ResIdx, Cycles);
+      unsigned NRCycle, InstanceID;
+      std::tie(NRCycle, InstanceID) =
----------------
InstanceID unused?


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:2202
+  unsigned NextAvailable, InstanceID;
+  std::tie(NextAvailable, InstanceID) =
+    getNextResourceCycle(PIdx, ResCount, Cycles);
----------------
InstanceID only used with DEBUG?


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:2207
                       << SchedModel->getProcResource(PIdx)->Name
+                      << " instance " << InstanceID
                       << " reserved until @" << NextAvailable << "\n");
----------------
Can we convert the instanceID to a resource name?


================
Comment at: llvm/test/CodeGen/ARM/proc-resource-sched.ll:15
+
+; Cortex-R52 model describes it as dual-issue with two integer ALUs
+; It should be able to issue the two additions in the same cycle.
----------------
The function is quite small, can you check the full schedule?


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

https://reviews.llvm.org/D51160