This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Introduce RC flags for vector register classes
ClosedPublic

Authored by cdevadas on Aug 27 2021, 4:32 AM.

Details

Summary

Configure and use the TSFlags in TargetRegisterClass to
have unique flags for VGPR and AGPR register classes.
The vector register class queries like hasVGPRs will
now become more efficient with just a bitwise operation.

Diff Detail

Event Timeline

cdevadas created this revision.Aug 27 2021, 4:32 AM
cdevadas requested review of this revision.Aug 27 2021, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2021, 4:32 AM
rampitec added inline comments.Aug 27 2021, 11:54 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
312

Aren't TSFlags initialized to 0 by default? I think you can drop most of the changes to use SIRegisterClass and only keep it for VGPRs and AGPRs.

cdevadas added inline comments.Aug 27 2021, 5:06 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
312

Yes, the bit vector is initialized to zero at its declaration statement in Target.td.

I thought having SIRegisterClass everywhere would make it simpler for any additional flags set in the future.
I could otherwise leave a comment where I define SIRegisterClass to indicate it and currently use it only for vector RCs.

rampitec added inline comments.Aug 27 2021, 5:11 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
312

No strong opinion, but I'd prefer a smaller diff.

cdevadas updated this revision to Diff 369241.Aug 27 2021, 11:23 PM
cdevadas edited the summary of this revision. (Show Details)

SIRegisterClass interface only for the reg classes that use TSFlags.

rampitec added inline comments.Aug 30 2021, 10:01 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
851

This is suspicious. I wonder why SIRegisterInfo::isVGPR() works. Probably because VS classes are unallocatable and it is called only after selection when we do not have unallocatable classes anymore.

Anyway, using this bit on a combined classes seems misleading. Maybe it shall be named 'HasVGPRs' instead?

cdevadas added inline comments.Aug 30 2021, 10:00 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
851

Anyway, using this bit on a combined classes seems misleading. Maybe it shall be named 'HasVGPRs' instead?

Yes, HasVGPRs flag makes more sense for the combined classes. I will update the patch.

851

This is suspicious. I wonder why SIRegisterInfo::isVGPR() works. Probably because VS classes are unallocatable and it is called only after selection when we do not have unallocatable classes anymore.

Right, it is possible that SIRegisterInfo::isVGPR() can identify it as an exclusive vgpr class. It is true even with the upstream compiler today. Maybe we don't have a test or a use case to catch it. The unallocatable classes do appear in the register-class query during selection. Do you think it is worth having another flag for SGPRs?

rampitec added inline comments.Aug 31 2021, 9:51 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
2169–2170

I think we need a comment here it only works with an allocatable class until we need to change it to work with combined classes.

llvm/lib/Target/AMDGPU/SIRegisterInfo.td
851

It might be beneficial to add an SGPR flag, but not necessarily now. I think all of that will need a revisit when a first combined class will become allocatable. For now a rename of flags shall be OK.

rampitec added inline comments.Aug 31 2021, 10:28 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
2169–2170

Not here, actually. That is isVGPRClass would be broken if given a VS class. So it seems we will need a flag for SGPRs and check all 3 flags. Not in this patch.

cdevadas updated this revision to Diff 369743.Aug 31 2021, 11:01 AM

Addressed the review comments.

This revision is now accepted and ready to land.Aug 31 2021, 11:52 AM
This revision was landed with ongoing or failed builds.Sep 1 2021, 12:04 AM
This revision was automatically updated to reflect the committed changes.