[PATCH] D80713: [AMDGPU] Support disassembly for AMDGPU kernel descriptors

[Ronak Chauhan via Phabricator via llvm-commits]

rochauha added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1243-1250
+  uint32_t NextFreeVGPR = 0;
+  if (isGFX9()) {
+    NextFreeVGPR = (GranulatedWorkitemVGPRCount + 1) * 4;
+  } else if (isGFX10()) {
+    bool IsWave64 = STI.getFeatureBits()[AMDGPU::FeatureWavefrontSize64];
+    uint32_t Tmp = GranulatedWorkitemVGPRCount + 1;
+    NextFreeVGPR = IsWave64 ? Tmp * 4 : Tmp * 8;
----------------
scott.linder wrote:
> I think this can just be replaced with:
> 
> ```
> NextFreeVGPR = (GranulatedWorkitemVGPRCount + 1) * getVGPRAllocGranule(STI);
> ```
> 
> Or we could add another function called `getNumVGPRs(const MCSubtargetInfo *STI, unsigned NumVGPRBlocks, Optional<bool> EnableWavefrontSize32)` and put the definition directly next to `getNumVGPRBlocks(const MCSubtargetInfo *STI, unsigned NumVGPRs, Optional<bool> EnableWavefrontSize32)` so any future changes affecting one also affect the other. I would lean towards this, and documenting that they are the inverse of one another.
I think this route is good for now :
NextFreeVGPR = (GranulatedWorkitemVGPRCount + 1) * getVGPRAllocGranule(STI);

Similarly for SGPRs.

I'd like to add the new functions via a separate patch. This patch is already quite big in terms of size.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1278-1285
+  if (isGFX9()) {
+    NextFreeSGPR = ((GranulatedWavefrontSGPRCount / 2) + 1) * 16;
+  } else if (isGFX10() && GranulatedWavefrontSGPRCount) {
+    return MCDisassembler::Fail;
+  } else {
+    // GFX 6-8
+    NextFreeSGPR = (GranulatedWavefrontSGPRCount + 1) * 8;
----------------
scott.linder wrote:
> Same as above, I think a new `getNumSGPRs` to complement `getNumSGPRBlocks` would make this easier to read.
> 
> For the GFX10 case we could either leave the check here, or have the new function return `Optional` to indicate when there is an error.
As I mentioned in my other reply, this patch is already quite big. So it'd be better to have a separate patch for the new functions.


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:1290
+  KdStream << Indent << ".amdhsa_reserve_xnack_mask " << 0 << '\n';
+  // KdStream << "GS = " << GranulatedWavefrontSGPRCount << "\n";
+  KdStream << Indent << ".amdhsa_next_free_sgpr " << NextFreeSGPR << "\n";
----------------
scott.linder wrote:
> What is "GS" and why is this commented out?
This was was for printing **G**ranulated wave front **S**GPR count.


================
Comment at: llvm/test/tools/llvm-objdump/ELF/AMDGPU/kernel-descriptor.s:6
+
+.amdhsa_kernel my_kernel
+.amdhsa_next_free_vgpr 2
----------------
scott.linder wrote:
> Can you implement more tests? I don't know if it is feasible to include them all in the same .s file, as you have to work around the output not being re-assembleable in general, but even just copy-pasting and editing is fine with me. Just having some more test cases to cover at least a reasonable sample of the different branches, failure modes, etc.
I have added new tests in separate files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80713