[PATCH] D86836: Support a list of CostPerUse values

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet requested changes to this revision.
qcolombet added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:215
+  const bool
+      *InAllocatableClass; // Register belongs to an allocatable regclass.
 };
----------------
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?


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


================
Comment at: llvm/include/llvm/CodeGen/TargetRegisterInfo.h:343
+  /// returned by RegisterCostTableIndex.
+  ArrayRef<uint8_t> getRegisterCosts(const MachineFunction &MF) const {
+    unsigned Idx = getRegisterCostTableIndex(MF);
----------------
cdevadas wrote:
> qcolombet wrote:
> > Could we not expose this table and have the users stick to getCostPerUse instead?
> It's an overhead to get the index on every cost query with `getCostPerUse`. This will instead, help to extract out a slice of the regcost table (as returned by `getRegisterCosts`) dynamically once from the tablegened register-costs-table with the help of the target handler `getRegisterCostTableIndex`.
Fair enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86836