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

[Patrick Holland via Phabricator via llvm-commits]

holland11 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.

Yeah, this is actually what I would personally recommend. MCA is smart enough to detect hardware, register, and most memory dependencies. So even with the `RetireOOO` flag, instructions still won't be dispatched if there are any hazards. My (very naive) guess would be that you'd get much more accurate simulations with the RetireOOO flag set globally. I'll make this change and produce a couple examples and you can decide if you prefer it.

> "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.

Great! I figured it might be as simple as this. I'll make the change and update the diff.

> I don't think you'll ever see Pseudos in mca, will you? It should all be Real instructions.

AFAIU, many of the design decision within mca are made with the idea that mca could potentially end up being used in a backend pass one day. I can remove the pseudo cases if you'd like, but in the hypothetical future where mca gets used in a AMDGPU backend pass, you'd probably want to add them back in.

> Hmm, I wonder if you could examine SIInstrFlags::LGKM_CNT instead of having to list these opcodes explicitly?

I can make that change if you'd like, but those specific opcodes are checked within the `SIInsertWaitcnts::updateEventWaitcntAfter()` function which all of this is based on.

  else {
      switch (Inst.getOpcode()) {
      case AMDGPU::S_SENDMSG:
      case AMDGPU::S_SENDMSGHALT:
        ScoreBrackets->updateByEvent(TII, TRI, MRI, SQ_MESSAGE, Inst);
        break;
      case AMDGPU::S_MEMTIME:
      case AMDGPU::S_MEMREALTIME:
        ScoreBrackets->updateByEvent(TII, TRI, MRI, SMEM_ACCESS, Inst);
        break;
      }

The reason for checking these specific opcodes could just be because they give different 'event types' (`SQ_MESSAGE` vs `SMEM_ACCESS`). Both of those event types map to `lgkmcnt` so I have them bundled together, but if you want me to just change it to checking for the `LGKM_CNT` flag, then I can do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104730