[PATCH] D101633: [AMDGPU] Set number vgprs used in PS shaders based on input registers actually used

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1136
+    uint32_t InputAddr = 0;
+    unsigned LastEna;
+
----------------
Doesn't LastEna need to be initialized here too? I assume all these initializations are just to avoid "may be used uninitialized" warnings.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1140
+      InputEna = MFI->getPSInputEnable();
+      InputAddr = MFI->getPSInputEnable();
+      // We only need to consider input args up to the last used arg
----------------
Should this be getPSInputAddr, and if so why didn't the tests you added fail?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1142
+      // We only need to consider input args up to the last used arg
+      LastEna = findLastSet(InputEna) + 1;
+    }
----------------
Can InputEna ever be zero, and if so what should LastEna be?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1156
+        // If this is a PS shader and we're processing the PS Input args (first
+        // 16 VGPR) - use the InputEna and InputAddr bits to define how many
+        // VGPRs are actually used.
----------------
Comma instead of dash?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp:1180
     ProgInfo.NumSGPR = std::max(ProgInfo.NumSGPR, WaveDispatchNumSGPR);
-    ProgInfo.NumVGPR = std::max(ProgInfo.NumVGPR, WaveDispatchNumVGPR);
+    ProgInfo.NumArchVGPR = std::max(ProgInfo.NumVGPR, WaveDispatchNumVGPR);
+    ProgInfo.NumVGPR =
----------------
What effect does setting NumArchVGPR here have, given that it was not being set before?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.h:56
     int32_t getTotalNumSGPRs(const GCNSubtarget &ST) const;
+    int32_t getTotalNumVGPRs(const GCNSubtarget &ST, int32_t NumAGPR,
+                             int32_t NumVGPR) const;
----------------
This function (and possibly the ones next to it) could do with a comment. It's not obvious to me what TotalNumVGPRs means, given that one of the inputs is already called NumVGPR.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101633