This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add True16 register classes.
ClosedPublic

Authored by kosarev on Jul 24 2023, 3:54 AM.

Diff Detail

Event Timeline

kosarev created this revision.Jul 24 2023, 3:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 3:54 AM
kosarev requested review of this revision.Jul 24 2023, 3:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2023, 3:54 AM
foad added inline comments.Jul 24 2023, 6:30 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
593–594

Do VGPR_LO16 and VGPR_HI16 need to define BaseClassOrder? From the comments below it sounds like the base class of a 16-bit VGPR will always be VGPR_16_Lo128 or VGPR_16.

1035

Use Reg16Types.types here and below?

kosarev updated this revision to Diff 543590.Jul 24 2023, 9:29 AM

Removed extra BaseClassOrder assignments and use Reg16Types.types instead of [i16, f16].

kosarev marked 2 inline comments as done.Jul 24 2023, 9:30 AM
kosarev added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
593–594

Indeed these are now base classes to no registers. Nice catch, thanks.

rampitec added inline comments.Jul 24 2023, 10:39 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
1036

Does this really work when you have an RC with registers of different size?

kosarev marked an inline comment as done.Jul 25 2023, 6:49 AM
kosarev added inline comments.
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
1036

Does this really work when you have an RC with registers of different size?

It seems so, yes. Downstream I see us generating instructions like v_and_b16 v0.l, s2, v0.l without problem.

rampitec accepted this revision.Jul 25 2023, 12:21 PM

LGTM

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

OK, thanks! I used to have problems with this in the past, thus the question.

This revision is now accepted and ready to land.Jul 25 2023, 12:21 PM
Joe_Nash added inline comments.Jul 27 2023, 1:22 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
602

I think the comment downstream about which instruction types use which types of Registers is helpful.

llvm/test/CodeGen/AMDGPU/subreg-undef-def-with-other-subreg-defs.mir
31–33

Unrelated to this patch, but I don't understand the purpose of these tests. It looks like we are referencing register classes by number in the test input. Therefore any time we modify the number of register classes the tests changes their meaning.

arsenm added inline comments.Jul 27 2023, 1:40 PM
llvm/test/CodeGen/AMDGPU/subreg-undef-def-with-other-subreg-defs.mir
31–33

The raw number thing is a syntactical inconvenience from how inlineasm is represented. Ideally the mir printer/parser would produce something nicer. I think the purpose of the test is reasonably clear from the comment at the top

Joe_Nash added inline comments.Jul 27 2023, 2:07 PM
llvm/test/CodeGen/AMDGPU/subreg-undef-def-with-other-subreg-defs.mir
31–33

How can one determine if the change in output is still correct? The different RC choices in coalescer-early-clobber-subreg.mir are even more abstract. Is the RC to be ignored?

arsenm added inline comments.Jul 27 2023, 6:22 PM
llvm/test/CodeGen/AMDGPU/subreg-undef-def-with-other-subreg-defs.mir
31–33

the printed class name should be correct, you just get the added bonus random number. Fur the purpose of this test the class doesn't really matter. However there are no 16-bit values in this test, so I don't understand how this ended up with any vgpr16 references in the first place

kosarev updated this revision to Diff 545172.Jul 28 2023, 8:19 AM

Added comments for the VGPR_16 and VGPR_16_Lo128 definitions.

kosarev added inline comments.Aug 1 2023, 3:53 AM
llvm/test/CodeGen/AMDGPU/subreg-undef-def-with-other-subreg-defs.mir
31–33

The InlineAsm instruction uses the numeric register class ids when computing these flags for register operands. These ids may change as we add or remove register classes. D78088 made the meaning of these flags visible in MIR tests, but there is still no support on the parser side for such operands.

Joe_Nash added inline comments.Aug 1 2023, 6:58 AM
llvm/test/CodeGen/AMDGPU/subreg-undef-def-with-other-subreg-defs.mir
31–33

These ids may change as we add or remove register classes.

Exactly.
The IDs in the input have not been updated but the output has been auto-updated about 10 times when someone previously added/removed a register class. Originally I think the register class was VRegOrLds_32. That is why we see 16-bit register classes in a test without 16-bit values. IMO this test format is not maintainable.

This revision was automatically updated to reflect the committed changes.