This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add 224-bit vector types and link 192-bit types to MVTs
ClosedPublic

Authored by critson on Jun 21 2021, 1:16 AM.

Details

Summary

Add SReg_224, VReg_224, AReg_224, etc.
Link 224-bit types with v7i32/v7f32.
Link existing 192-bit types to newly added v3i64/v3f64/v6i32/v6f32.

Diff Detail

Event Timeline

critson created this revision.Jun 21 2021, 1:16 AM
critson requested review of this revision.Jun 21 2021, 1:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 1:16 AM
rampitec accepted this revision.Jun 21 2021, 10:28 AM

LGTM, although v3 change can have unexpected impact. Could you please run PSDB on this?

This revision is now accepted and ready to land.Jun 21 2021, 10:28 AM
foad added inline comments.Jun 22 2021, 1:48 AM
llvm/test/CodeGen/AMDGPU/sdiv64.ll
502

This looks like a minor regression. Why are we still loading s0? Maybe not worth holding up the patch for though.

LGTM, although v3 change can have unexpected impact. Could you please run PSDB on this?

To confirm, PSDB and other GFX related test pass.

foad added inline comments.Jun 23 2021, 5:28 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
739–746

Can't we get rid of the dual SGPR_*/SReg_* classes for all sizes > 64? I thought the only difference between them was that Sreg_* added non-SGPR registers like EXEC and M0, but for the larger sizes, if there are no non-SGPR registers to add, what is the point of having two different classes?

arsenm added inline comments.Jun 23 2021, 5:28 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
739–746

No, because the TTMP_* registers also form the large tuples

foad added inline comments.Jun 23 2021, 5:32 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
739–746

Right but currently TTMP_160, TTMP_192 etc are not defined. Maybe they should be, and the SReg_* classes should include them, and then we wouldn't be in this ambiguous situation where two classes have the same size so tablegen picks one of them arbitrarily?

arsenm added inline comments.Jun 23 2021, 5:33 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
739–746

Oh, I think the confusion is there isn't actually a set of SGPR_* classes. That's just the name of the set of registers added to the class. It would be better if the classes were named SGPR_* and we had the SReg_* classes for the addition of special regs

foad added inline comments.Jun 23 2021, 5:37 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
739–746

We have things like:

def SGPR_192 : RegisterClass<"AMDGPU", [untyped], 32, (add SGPR_192Regs)> {
  let Size = 192;
  let AllocationPriority = 17;
}

So SGPR_192 is a class and SGPR_192Regs is the set of regs. I'm not sure how consistent this is for the other sizes though.

arsenm added inline comments.Jun 23 2021, 5:39 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.td
739–746

Oh, I was thinking of VGPRs which is for some reason different. It always annoys me how you have to use VGPR_32 but everything higher is VReg_*

This revision was landed with ongoing or failed builds.Jun 23 2021, 8:41 PM
This revision was automatically updated to reflect the committed changes.

I have committed this but I am happy to add the TTMP classes as a follow up.
I was unsure whether it was appropriate to add more classes that probably are not even used?