This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Adapt GCNRegBankReassign for 16 bit subregs
ClosedPublic

Authored by rampitec on Apr 23 2020, 4:35 PM.

Details

Summary

It allows it not to crash and analyze 16 bit subregs if those
appear in the instructions. At the same time it does not attempt
to reassign these. It still can correctly identify register
banks to let larger registers to be reassigned.

More work will be needed here when real instructions will use
these registers and more tests as well.

Diff Detail

Event Timeline

rampitec created this revision.Apr 23 2020, 4:35 PM
rampitec edited the summary of this revision. (Show Details)Apr 23 2020, 4:47 PM
foad added a subscriber: foad.Apr 24 2020, 3:02 AM

s/Adopt/Adapt/ :-)

rampitec retitled this revision from [AMDGPU] Adopt GCNRegBankReassign for 16 bit subregs to [AMDGPU] Adapt GCNRegBankReassign for 16 bit subregs.Apr 24 2020, 10:33 AM
rampitec updated this revision to Diff 259942.Apr 24 2020, 11:14 AM

Added interference test.

arsenm added inline comments.Apr 24 2020, 11:28 AM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
456–457

This kills the code below

463

Can't you just check SGPR16_LO.contains instead of going through the size check? I would expect you can get rid of the explicit size check by just trying getSubReg/getSuperReg and see if either failed

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1789–1790

Seems like this should just be an assert

llvm/test/CodeGen/AMDGPU/regbank-reassign.mir
374–378

If you don't need the preferred-register hints you can eliminate the registers section

rampitec updated this revision to Diff 259953.Apr 24 2020, 12:16 PM
rampitec updated this revision to Diff 259955.
rampitec marked 5 inline comments as done.

Uploaded correct patch.

rampitec added inline comments.Apr 24 2020, 12:19 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
456–457

Yes, and it has TODO above it. These registers are still reassignable, it just needs more code.

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1789–1790

I am going to use it without checking is a register actually 16 or 32 bit, just to get a 32 bit operand from whatever input. But it is reasonable to add assert for "Size <= 32".

llvm/test/CodeGen/AMDGPU/regbank-reassign.mir
374–378

I intend to reuse these tests when registers become reassignable. I also want to trigger code path in the pass which will actually go through the bank search logic. If RA will just assign v0 and v1 not all code would trigger.

arsenm added inline comments.Apr 24 2020, 4:01 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1789–1790

I think you can get away with looking up the specific register class, and picking the right 32-bit class by just trying VGPR,SGPR,AGPR in succession and see if any succeed

llvm/lib/Target/AMDGPU/SIRegisterInfo.h
287–291

I would expect this to be an assert, but. guess it already handled this case

rampitec marked an inline comment as done.Apr 24 2020, 4:19 PM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.h
287–291

That's because hi16 returns 16 as an offset, and then divideCell returns 1 which is plain wrong.

rampitec marked an inline comment as done.Apr 24 2020, 4:21 PM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
1789–1790

I could but I don't think it is faster.

rampitec updated this revision to Diff 260418.Apr 27 2020, 12:56 PM
rampitec marked an inline comment as done.
  • Scan applicable RC with getMatchingSuperReg in get32BitRegister().
  • Removed divideCell from getChannelFromSubReg() to simplify it.
Herald added a project: Restricted Project. · View Herald TranscriptApr 27 2020, 12:56 PM
rampitec updated this revision to Diff 260449.Apr 27 2020, 1:41 PM

Only check hi16 for VGPRs, the rest are artificial anyway.

arsenm added inline comments.Apr 28 2020, 12:43 PM
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
462

Does this need to be updated for SReg_Lo16 in the other patch?

rampitec marked an inline comment as done.Apr 28 2020, 1:00 PM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/GCNRegBankReassign.cpp
462

No, pass does not reassign special SGPRs. In essence you cannot replace vcc with s[0:1] here.

arsenm accepted this revision.Apr 28 2020, 1:31 PM
This revision is now accepted and ready to land.Apr 28 2020, 1:31 PM
This revision was automatically updated to reflect the committed changes.
foad added inline comments.May 27 2020, 1:24 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.h
287–291

divideCeil(16, 32) returns 1 which is plain right!

rampitec marked an inline comment as done.May 27 2020, 8:42 AM
rampitec added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.h
287–291

It is channel 0 and not 1, and it was a bug.