[PATCH] D104730: [MCA] [AMDGPU] Adding CustomBehaviour implementation for AMDGPU.

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

Thanks for doing this! Generally it seems very useful, and my only high level concern is about the way you set RetireOOO //only// on s_waitcnt instructions. Would it make more sense to set it for all AMDGPU instructions? I'm really not sure what effect this would have. I've never really thought about the concept of "retiring" in our sched model, since it doesn't seem to have any impact on how you should schedule instructions.

To respond to some of your specific points:

> Two of the functions that I couldn't recreate are related to memory operands and I'm not entirely sure if it's possible to recreate those functions using MCInst rather than the MachineInstr that are used in the pass this is all based on.

It's not. The memoperands carry higher level semantic information about pointer operands which simply is not (and should not be) available when you're working at the lower MCInst level.

> So if anyone knows how I can recreate the { return getGeneration() < SEA_ISLANDS; } behaviour using either a AMDGPU::IsaVersion or a MCSubtargetInfo, I'd be happy to update this diff before it gets committed

"Sea Islands" is ISA version 7 (don't be misled by the enumerator SEA_ISLANDS having the value 6 :-/ ) so you should be able to implement it as `IsaVersion.Major < 7`.

> I'm not sure how the s_waitcnt_depctr instruction works. I couldn't find any information within the ISAs, so I just didn't implement anything for this instruction.

It tests some other counters, completely unrelated to normal s_waitcnt, so you are right to ignore it.



================
Comment at: llvm/tools/llvm-mca/lib/AMDGPU/AMDGPUCustomBehaviour.cpp:25-30
+  case AMDGPU::S_WAITCNT:
+  case AMDGPU::S_WAITCNT_DEPCTR:
+  case AMDGPU::S_WAITCNT_EXPCNT:
+  case AMDGPU::S_WAITCNT_LGKMCNT:
+  case AMDGPU::S_WAITCNT_VMCNT:
+  case AMDGPU::S_WAITCNT_VSCNT:
----------------
I don't think you'll ever see Pseudos in mca, will you? It should all be Real instructions.


================
Comment at: llvm/tools/llvm-mca/lib/AMDGPU/AMDGPUCustomBehaviour.cpp:75-80
+  case AMDGPU::S_WAITCNT: // This instruction
+  case AMDGPU::S_WAITCNT_DEPCTR:
+  case AMDGPU::S_WAITCNT_EXPCNT:
+  case AMDGPU::S_WAITCNT_LGKMCNT:
+  case AMDGPU::S_WAITCNT_VMCNT:
+  case AMDGPU::S_WAITCNT_VSCNT: // to this instruction are all pseudo.
----------------
Likewise, no need to handle Pseudos?


================
Comment at: llvm/tools/llvm-mca/lib/AMDGPU/AMDGPUCustomBehaviour.cpp:291-294
+      case AMDGPU::S_SENDMSG:
+      case AMDGPU::S_SENDMSGHALT:
+      case AMDGPU::S_MEMTIME:
+      case AMDGPU::S_MEMREALTIME:
----------------
Hmm, I wonder if you could examine SIInstrFlags::LGKM_CNT instead of having to list these opcodes explicitly?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104730