[PATCH] D86836: Support a list of CostPerUse values

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

LGTM, just make sure to move `getRegisterCostTableIndex` under protected.



================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:215
+  const bool
+      *InAllocatableClass; // Register belongs to an allocatable regclass.
 };
----------------
cdevadas wrote:
> 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.
Got it, thanks for the detailed explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86836