[PATCH] D116140: [AMDGPU] Add agpr_count to metadata and AsmParser

[Jacob Lambert via Phabricator via llvm-commits]

lamb-j added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:1168
+
+  // Sourced from AMDGPUResourceUsageAnalysis::SIFunctionResourceInfo::getTotalNumVGPRs()
+  int getTotalNumVgprs() {
----------------
arsenm wrote:
> Really should avoid repeating this then. Usually things that need to be shared between codegen and MC go in AMDGPUBaseInfo
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116140/new/

https://reviews.llvm.org/D116140