This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU][NFC] Refactor AMDGPUCallingConv.td
ClosedPublic

Authored by scott.linder on Aug 31 2021, 11:29 AM.

Details

Summary

Rename CalleeSavedRegs defs to avoid being overly specific:

  • CSR_AMDGPU_AGPRs_32_255 => CSR_AMDGPU_AGPRs
  • CSR_AMDGPU_SGPRs_30_31 + CSR_AMDGPU_SGPRs_32_105 => CSR_AMDGPU_SGPRs
  • CSR_AMDGPU_SI_Gfx_SGPRs_4_29 + CSR_AMDGPU_SI_Gfx_SGPRs_64_105 => CSR_AMDGPU_SI_Gfx_SGPRs
  • CSR_AMDGPU_HighRegs => CSR_AMDGPU
  • CSR_AMDGPU_HighRegs_With_AGPRs => CSR_AMDGPU_GFX90AInsts
  • CSR_AMDGPU_SI_Gfx_With_AGPRs => CSR_AMDGPU_SI_Gfx_GFX90AInsts

Introduce a class RegMask to mark the cases where we use the
CalleeSavedRegs class purely as an expedient way to produce a mask.
Update the names of these masks to not mention "CSR". Other targets also
seem to do this, so a reasonable alternative is to actually update
table-gen to include a new class to do this explicitly, but the current
approach seems harmless so I opted to just make it more explicit.

Diff Detail

Event Timeline

scott.linder created this revision.Aug 31 2021, 11:29 AM
scott.linder requested review of this revision.Aug 31 2021, 11:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2021, 11:29 AM
arsenm added inline comments.Aug 31 2021, 11:38 AM
llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td
147

It's not entirely accurate since amdgpu_gfx will use the same masks

230

Probably shouldn't have the CSR prefix if it's just all registers

llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
352–353

Changing the convention based on the subtarget seems problematic. Do we really need to do this?

rampitec added inline comments.Aug 31 2021, 11:58 AM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
352–353

There is no good way to spill AGPRs on gfx908. On gfx90a you can use it in ld/st.

arsenm added inline comments.Aug 31 2021, 12:05 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
352–353

Then why not just treat them as scratch registers unconditionally?

rampitec added inline comments.Aug 31 2021, 12:09 PM
llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp
352–353

To give gfx90a a way to use mfma without too much overhead... But maybe really just do it the same way on both.

scott.linder added inline comments.Sep 1 2021, 12:43 PM
llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td
147

Ah, fair, I can remove the headings if they don't add any value

230

I had originally used a different prefix, and considered no prefix, but I wasn't sure if it would be confusing enough in e.g. the MIR to have some masks with the prefix and some without? Is there any risk of colliding with something else in the MIR syntax?

scott.linder added inline comments.Sep 1 2021, 1:17 PM
llvm/lib/Target/AMDGPU/AMDGPUCallingConv.td
230

I looked in the MIRParser and it doesn't seem like stripping the prefix from these should cause any ambiguity:

    case MIToken::Identifier:
      if (const auto *RegMask = PFS.Target.getRegMask(Token.stringValue())) {
        Dest = MachineOperand::CreateRegMask(RegMask);
        lex();
        break;
      } else if (Token.stringValue() == "CustomRegMask") {
        return parseCustomRegisterMaskOperand(Dest);
      } else
        return parseTypedImmediateOperand(Dest);

...

  bool MIParser::parseTypedImmediateOperand(MachineOperand &Dest) {
    assert(Token.is(MIToken::Identifier));
    StringRef TypeStr = Token.range();
    if (TypeStr.front() != 'i' && TypeStr.front() != 's' &&
        TypeStr.front() != 'p')
      return error(
          "a typed immediate operand should start with one of 'i', 's', or 'p'");

Even if we collided with the prefixes used for immediates the parser prioritizes regmasks.

I'll remove the prefix from these and post an updated patch

scott.linder edited the summary of this revision. (Show Details)Sep 2 2021, 12:45 PM
scott.linder edited the summary of this revision. (Show Details)

Remove misleading comment headers, strip "CSR_" prefix from regmask identifiers.

You probably need to rebase this on top of D111637

arsenm accepted this revision.Apr 6 2022, 7:25 PM

LGTM

This revision is now accepted and ready to land.Apr 6 2022, 7:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 7:25 PM
Herald added a subscriber: hsmhsm. · View Herald Transcript
scott.linder edited the summary of this revision. (Show Details)

Rebase

arsenm accepted this revision.Apr 12 2022, 1:42 PM
sebastian-ne accepted this revision.Apr 19 2022, 1:36 AM

LGTM, nice!

This revision was landed with ongoing or failed builds.Jun 1 2022, 9:25 AM
This revision was automatically updated to reflect the committed changes.