Page MenuHomePhabricator

Support a list of CostPerUse values
Needs ReviewPublic

Authored by cdevadas on Aug 29 2020, 5:23 AM.



This patch allows targets to define multiple cost
values for each register so that the cost model
can be more flexible and better used during the
register allocation as per the target requirements.

For AMDGPU the VGPR allocation will be more efficient
if the register cost can be associated dynamically
based on the calling convention.

Diff Detail

Event Timeline

cdevadas created this revision.Aug 29 2020, 5:23 AM
cdevadas requested review of this revision.Aug 29 2020, 5:23 AM
madhur13490 added inline comments.

There is an implicit upcast happening here from uint8_t to unsigned. This may lead to unexpected behavior on different platform, I'd recommend to be consistent unless absolutely necessary to use unsigned.


Is there any specific reason to use int64_t type for CostPerUse?


This block of comment can be split and put above the relevant line for better readability. Having a block of comment doesn't really say which logical block of code is meant for.


Probably a better name for RegCosts. Something like AllRegCostPerUse.


I think you can use std::bitset or llvm::BitVector here. vector<bool> is less space optimal than these.


What is the purpose of this extra block of 0s at the beginning?

cdevadas added inline comments.Aug 31 2020, 3:12 AM

Will change such instances to uint8_t.


getValueAsListOfInts works only with int64_t type.


It is for NoRegister which is the first entry in the generated register list

cdevadas updated this revision to Diff 288909.Aug 31 2020, 3:15 AM

Addressed the review comments.

asb added a comment.Sep 3 2020, 4:49 AM

It looks like others are reviewing the implementation details, but I just wanted to chime in to say that this looks like a useful feature for the RISC-V backend too. Currently we set CostPerUse for some registers in order to tweak register allocation in a way that benefits codegen when the compressed instruction set extension is enabled. Although the impact is _very_ minimal, this slightly harms targets that don't support the compressed instruction set (admittedly, this isn't really true of any shipping RISC-V processor so not a big concern). It would be good to use this vary the CostPerUse depending on target RISC-V features.

Ping @stoklund @MatzeB or anyone interested in this patch.
Appreciate it if this gets reviewed soon.

cdevadas updated this revision to Diff 296621.Oct 7 2020, 2:02 AM
cdevadas added reviewers: qcolombet, wmi.

rebase + ping

qcolombet requested changes to this revision.Oct 9 2020, 12:34 PM


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/ for instance.



Why do we need this array?
The information should be readily available through TRI->getCostPerUse() (maybe by adding an MF argument).


Could we stick to unsigned here to avoid leaking the implementation details of MinCost?


Again could we stick to using getCostPerUse everywhere?


The allocatable part cannot change, can it?
I.e., why is this an array?


This should be protected, no user should care about that.


Could we not expose this table and have the users stick to getCostPerUse instead?


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.


Is there a way to use a type that enforce that instead of just writing it in a comment?


Why can't we channel the new information through CostPerUse instead of adding a new API?


Ditto about leaking the implementation details of something we shouldn't care (here MinCost).


Ditto why can't we use getCostPerUse here?

This revision now requires changes to proceed.Oct 9 2020, 12:34 PM
cdevadas added inline comments.Oct 11 2020, 7:17 AM

Will do.


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.


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.


I don't think Tablegen have different sized int types.

cdevadas added inline comments.Oct 16 2020, 12:40 AM

MinCost is actually declared as uint8_t in RegisterClassInfo.h and I think it is good to keep it uint8_t everywhere.
The comparison with CostPerUseLimit in the next line makes it all consistent since I reduced the reg-cost type to uint8_t with this patch.

cdevadas updated this revision to Diff 302237.Nov 2 2020, 3:29 AM

Added testcases.

cdevadas updated this revision to Diff 305048.Fri, Nov 13, 12:08 AM

rebase + elaborated the comment in

lenary removed a subscriber: lenary.Fri, Nov 13, 3:56 AM

@qcolombet can you review this patch?