[PATCH] D51160: Adjust MIScheduler to use ProcResource counts

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:686
+  // it.
+  SmallVector<unsigned, 16> ProcessorResourceList;
+
----------------
fhahn wrote:
> This name is too generic I think. this is only relevant to ReservedCycles. Also it is basically a map, right?
I agree: the name should be improved.
`ProcessorResourceList` is only used to map processor resource identifiers to ReservedCycles indices. That being said, it is okay to have it as a small vector with size `SchedModel->getNumProcResourceKinds()`.


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

I don't think it is really needed (see my other comment below).


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:1930-1933
+    for (unsigned i = 0; i < ResourceCount; ++i) {
+      ProcessorResourceList[i] = NumUnits;
+      NumUnits += (Resources + i)->NumUnits;
+    }
----------------
Instead of doing `(Resources + i)->NumUnits`, you should use
`SchedModel->getProcResource(i)` to ge the MCProcResourceDesc, and then access field `NumUnits`.

So, something like:
```
NumUnits += SchedModel->getProcResource(i)->NumUnits;
```

That way, you can get rid of the code at line 1924. You also don't need method `getProcResourceTable()`.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:1979-1980
+  unsigned NumberOfInstances = SchedModel->getProcResource(PIdx)->NumUnits;
+  assert(NumberOfInstances > 0 &&
+         "Cannot have zero instances of a ProcResource");
+
----------------
We should probably enforce this constraint in the SubtargetEmitter and error out if a processor resource declares zero (or negative) units.

(In practice, none of the models available upstream declares a resource with zero units. Only the 'InvalidUnit' at index 0 defaults to having zero units).


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

https://reviews.llvm.org/D51160