This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add agpr_count to metadata and AsmParser
ClosedPublic

Authored by lamb-j on Dec 21 2021, 8:58 PM.

Details

Summary

gfx90a allows the number of ACC registers (AGPRs) to be set
independently to the VGPR registers. For both HSA and PAL metadata, we
now include an "agpr_count" key to report the number of AGPRs set for
supported devices (gfx90a, gfx908, as determined by hasMAIInsts()).
This is collected from SIProgramInfo.NumAccVGPR for both HSA and PAL.
The AsmParser also now recognizes ".kernel.agpr_count" for supported
devices.

Diff Detail

Unit TestsFailed

Event Timeline

lamb-j created this revision.Dec 21 2021, 8:58 PM
lamb-j requested review of this revision.Dec 21 2021, 8:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 21 2021, 8:58 PM

Add @tpr to look at pal metadata changes

arsenm added inline comments.Dec 22 2021, 8:17 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1158

Formatting and weird consta placement

1162

Ditto

1179

Fallthrough here for other targets

lamb-j updated this revision to Diff 395899.Dec 22 2021, 10:25 AM

Removing const from MCSymbol variable and creating fall through case for getTotalNumVgprs()

lamb-j marked 3 inline comments as done.Dec 22 2021, 10:27 AM
lamb-j added inline comments.
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1158

The const does seem to be unnecessary. Should I remove from lines 1134 and 1144 as well, or leave that for another patch?

lamb-j marked an inline comment as done and an inline comment as not done.Jan 13 2022, 11:22 AM
arsenm added inline comments.Jan 17 2022, 5:12 PM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1168

Really should avoid repeating this then. Usually things that need to be shared between codegen and MC go in AMDGPUBaseInfo

lamb-j added inline comments.Jan 20 2022, 8:09 AM
llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
1168

Ok perfect, I was looking for a good way to do this. So would you recommend lifting the implementation of getTotalNumVGPRs from AMDGPUResourceUsageAnalysis.cpp into Utils/AMDGPUBaseInfo.cpp, possibly as a separate patch?

Moreover, there’s actually already a function defined as “getTotalNumVGPRs” in Utils/AMDGPUBaseInfo.cpp, but its implementation is different than what we’re looking for in the AsmParser. Here are the two overlapping functions.

AMDGPUResourceUsageAnalysis.cpp
Returns a variable value that represents the number of VGPRs and AGPRs employed by a code region.

int32_t AMDGPUResourceUsageAnalysis::SIFunctionResourceInfo::getTotalNumVGPRs(
const GCNSubtarget &ST, int32_t ArgNumAGPR, int32_t ArgNumVGPR) const {
  if (ST.hasGFX90AInsts() && ArgNumAGPR)
    return alignTo(ArgNumVGPR, 4) + ArgNumAGPR;
  return std::max(ArgNumVGPR, ArgNumAGPR);
}

Utils/AMDGPUBaseInfo.cpp
Returns a constant value indicating the total number of physical VGPRs accessible on a device.

unsigned getTotalNumVGPRs(const MCSubtargetInfo *STI) {
  if (STI->getFeatureBits().test(FeatureGFX90AInsts))
    return 512;
  if (!isGFX10Plus(*STI))
    return 256;
  return STI->getFeatureBits().test(FeatureWavefrontSize32) ? 1024 : 512;
}

Are the overlapping names here intentional? Would it make sense to rename one of these in a separate patch, for example the AMDGPUResourceUsageAnalysis one to “getTotalNumUsedVGPRs” or something similar?

lamb-j updated this revision to Diff 405139.Feb 1 2022, 7:43 PM

Fixing MCSymbol const declaration formatting

lamb-j updated this revision to Diff 409363.Feb 16 2022, 12:23 PM

Switching to use new getTotalNumVGPRs from AMDGPUBaseInfo

lamb-j marked an inline comment as done.Feb 16 2022, 12:24 PM
lamb-j updated this revision to Diff 409364.Feb 16 2022, 12:25 PM

Updating commit message

arsenm accepted this revision.Feb 16 2022, 2:11 PM
This revision is now accepted and ready to land.Feb 16 2022, 2:11 PM
This revision was landed with ongoing or failed builds.Feb 16 2022, 3:17 PM
This revision was automatically updated to reflect the committed changes.