Page MenuHomePhabricator

[AMDGPU][NFC] Refactor AMDGPUCallingConv.td
Needs ReviewPublic

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

Details

Summary

Remove unused CalleeSavedRegs defs CSR_AMDGPU_VGPRs_24_255 and
CSR_AMDGPU_VGPRs_32_255 which appear to be leftovers from old iterations
of the calling convention.

Rename CalleeSavedRegs defs to avoid being overly specific:

  • CSR_AMDGPU_AGPRs_32_255 => CSR_AMDGPU_AGPRs
  • CSR_AMDGPU_SGPRs_32_105 => CSR_AMDGPU_SGPRs
  • CSR_AMDGPU_HighRegs => CSR_AMDGPU
  • CSR_AMDGPU_HighRegs_With_AGPRs => CSR_AMDGPU_GFX90AInsts

Introduce a class RegMask to mark the cases where we use the
CalleeSavedRegs class purely as an expedient way to produce a mask.
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