[PATCH] D86836: Support a list of CostPerUse values

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:215
+  const bool
+      *InAllocatableClass; // Register belongs to an allocatable regclass.
 };
----------------
qcolombet wrote:
> cdevadas wrote:
> > qcolombet wrote:
> > > The allocatable part cannot change, can it?
> > > I.e., why is this an array?
> > I have changed the way tablegen integrates these values to TargetRegisterInfo. It was an array of struct earlier and now a pointer to a table for the benefit of supporting the list of register costs. I thought it could be ok to change InAllocatableClass because it is part of TargetRegisterInfoDesc class. Otherwise, one of these members should be moved out of the class.
> > 
> I don't really see the point here.
> I initially thought it was to save space by sharing the boolean array with all instances of TargetRegisterInfoDesc, but we actually trade a bool with a pointer.
> 
> I don't understand the argument about having to move the members out of the class.
> 
> Why can't we stick with the bool is here again?
Earlier, the `CostPerUse` and `InAllocatableClass` were tablegened with exactly numRegs entries each. 
We could accommodate them in a single table and use the register itself to index into this table to retrieve the members during RA.

With this patch, the tablegened `CostPerUse` will have numCosts*numRegs entries wherein the `InAllocatableClass` field will remain unchanged (exactly numRegs entries).
The mismatch in their entries is the first reason I have generated the tables differently by creating two separate tables and a TargetRegisterInfoDesc instance that holds these tables.
Furthermore, an appropriate cost model will be extracted out dynamically using the `getRegisterCostTableIndex` and `getRegisterCosts` APIs so that we can still use the register to index into this dynamic table.

We should examine the `XXXRegisterInfo.inc` file generated with and without this patch to fully understand the new tables and the reason why I have changed the `InAllocatableClass` to a pointer. 
If you look at the test I included in this patch, llvm/test/TableGen/RegisterInfoEmitter-regcost-list.td
There are 6 registers defined (s0 - s3 and d0 - d1) in the .td file and with the additional `NoRegister` there will be total 7 registers in the generated file.

I have included 2 cost models that resulted in 14 entries in the CostPerUseTable. 
static const uint8_t `CostPerUseTable[]` = {  0, 0, 0, 1, 1, 1, 1, 0, 0, 0, 0, 1, 2, 3, };

For cost model 0: During RA, the ArrayRef RegCosts will get {0, 0, 0, 1, 1, 1, 1}
For cost model 1: During RA, the ArrayRef RegCosts will get {0, 0, 0, 0, 1, 2, 3}

The InAllocatableClassTable will have 7 entries irrespective of the cost model. 
static const bool `InAllocatableClassTable[]` = {  false, true, true, true, true, true, true, };

Finally, the TargetRegisterInfoDesc instance to be used by the RA.
static const TargetRegisterInfoDesc `MyTargetRegInfoDesc` = { // Extra Descriptors
  CostPerUseTable, 2, InAllocatableClassTable};

Assume if there is only one cost model (cost model 0) in the input file and with the upstream compiler today, the tablegened register info will look like the following:
static const TargetRegisterInfoDesc `MyTargetRegInfoDesc[]` = { // Extra Descriptors
  { 0, false },
  { 0, true },
  { 0, true },
  { 1, true },
  { 1, true },
  { 1, true },
  { 1, true },
};
 
The array `MyTargetRegInfoDesc[]` generated with the upstream compiler would now become an object `MyTargetRegInfoDesc` that points to the individual tables.


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:339
+    return 0;
+  }
+
----------------
qcolombet wrote:
> qcolombet wrote:
> > This should be protected, no user should care about that.
> This was not addressed, was it?
Sorry, I missed it. Will move it under Protected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86836