[PATCH] D86836: Support a list of CostPerUse values

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added a comment.
This revision now requires changes to proceed.

Hi,

The general direction is fine, but IMHO there are needless code churn.
Why can't we stick to getCostPerUse instead of changing all the users?

Also, this needs a test case.
Take a look at `llvm/test/TableGen/GlobalISelEmitter.td` for instance.

Cheers,
-Quentin



================
Comment at: llvm/include/llvm/CodeGen/RegisterClassInfo.h:70
+  // The register cost values.
+  ArrayRef<uint8_t> RegCosts;
+
----------------
Why do we need this array?
The information should be readily available through TRI->getCostPerUse() (maybe by adding an MF argument).


================
Comment at: llvm/include/llvm/CodeGen/RegisterClassInfo.h:125
   /// the registers in getOrder(RC).
-  unsigned getMinCost(const TargetRegisterClass *RC) {
-    return get(RC).MinCost;
-  }
+  uint8_t getMinCost(const TargetRegisterClass *RC) { return get(RC).MinCost; }
 
----------------
Could we stick to unsigned here to avoid leaking the implementation details of MinCost?


================
Comment at: llvm/include/llvm/CodeGen/RegisterClassInfo.h:130
   /// All registers in getOrder(RC).slice(getLastCostChange(RC)) will have the
-  /// same cost according to TRI->getCostPerUse().
+  /// same cost according to RegCosts[Reg].
   unsigned getLastCostChange(const TargetRegisterClass *RC) {
----------------
Again could we stick to using getCostPerUse everywhere?


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:215
+  const bool
+      *InAllocatableClass; // Register belongs to an allocatable regclass.
 };
----------------
The allocatable part cannot change, can it?
I.e., why is this an array?


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:339
+    return 0;
+  }
+
----------------
This should be protected, no user should care about that.


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:343
+  /// returned by RegisterCostTableIndex.
+  ArrayRef<uint8_t> getRegisterCosts(const MachineFunction &MF) const {
+    unsigned Idx = getRegisterCostTableIndex(MF);
----------------
Could we not expose this table and have the users stick to getCostPerUse instead?


================
Comment at: llvm/include/llvm/Target/Target.td:173
+  // list, targets can have multiple cost values associated with each register
+  // and can use one over the other based on the target requirements.
+  // Restricted the cost type to uint8_t in the generated table. It will
----------------
Could you call out that targets have to override `TargetRegisterInfo::getRegisterCostTableIndex` to use the related cost model?


================
Comment at: llvm/include/llvm/Target/Target.td:173
+  // list, targets can have multiple cost values associated with each register
+  // and can use one over the other based on the target requirements.
+  // Restricted the cost type to uint8_t in the generated table. It will
----------------
qcolombet wrote:
> Could you call out that targets have to override `TargetRegisterInfo::getRegisterCostTableIndex` to use the related cost model?
The comment should also call out that the cost is filled with zeros for mismatching sizes of register costs.


================
Comment at: llvm/include/llvm/Target/Target.td:176
+  // considerably reduce the table size.
+  list<int> CostPerUse = [0];
 
----------------
Is there a way to use a type that enforce that instead of just writing it in a comment?


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:411
+  /// Function
+  ArrayRef<uint8_t> RegCosts;
+
----------------
Why can't we channel the new information through CostPerUse instead of adding a new API?


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:1129
     const TargetRegisterClass *RC = MRI->getRegClass(VirtReg.reg());
-    unsigned MinCost = RegClassInfo.getMinCost(RC);
+    uint8_t MinCost = RegClassInfo.getMinCost(RC);
     if (MinCost >= CostPerUseLimit) {
----------------
Ditto about leaking the implementation details of something we shouldn't care (here MinCost).


================
Comment at: llvm/lib/CodeGen/RegAllocGreedy.cpp:1149
     assert(PhysReg);
-    if (TRI->getCostPerUse(PhysReg) >= CostPerUseLimit)
+    if (RegCosts[PhysReg] >= CostPerUseLimit)
       continue;
----------------
Ditto why can't we use getCostPerUse here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86836