Page MenuHomePhabricator

Support a list of CostPerUse values
Needs ReviewPublic

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

Details

Summary

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.
llvm/lib/CodeGen/RegAllocGreedy.cpp
804

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.

llvm/utils/TableGen/CodeGenRegisters.h
154

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

llvm/utils/TableGen/RegisterInfoEmitter.cpp
1457

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.

1458

Probably a better name for RegCosts. Something like AllRegCostPerUse.

1459

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

1460

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

cdevadas added inline comments.Aug 31 2020, 3:12 AM
llvm/lib/CodeGen/RegAllocGreedy.cpp
804

Will change such instances to uint8_t.

llvm/utils/TableGen/CodeGenRegisters.h
154

getValueAsListOfInts works only with int64_t type.

llvm/utils/TableGen/RegisterInfoEmitter.cpp
1460

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

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

llvm/include/llvm/CodeGen/RegisterClassInfo.h
70

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

125

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

130

Again could we stick to using getCostPerUse everywhere?

llvm/include/llvm/CodeGen/TargetRegisterInfo.h
214

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

338

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

342

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

llvm/include/llvm/Target/Target.td
173

Could you call out that targets have to override TargetRegisterInfo::getRegisterCostTableIndex to use the related cost model?

173

The comment should also call out that the cost is filled with zeros for mismatching sizes of register costs.

176

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

llvm/lib/CodeGen/RegAllocGreedy.cpp
411

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

1137

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

1157

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
llvm/include/llvm/CodeGen/RegisterClassInfo.h
125

Will do.

llvm/include/llvm/CodeGen/TargetRegisterInfo.h
214

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.

342

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.

llvm/include/llvm/Target/Target.td
176

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

cdevadas added inline comments.Oct 16 2020, 12:40 AM
llvm/lib/CodeGen/RegAllocGreedy.cpp
1137

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 Target.td

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

@qcolombet can you review this patch?