[PATCH] D104293: [AMDGPU] Set more flags on Real instructions

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

> So basically just the TSFlags (assuming it's easier to copy them all at once rather than just the individual ones needed), mayLoad, and mayStore.

Hmm. These are not all being copied correctly at the moment. Unfortunately there is no easy way to check for discrepancies. I am knocking up a python script to do it based on the output of `llvm-tblgen -dump-json`.

> It seems as though the TSFlags were already being copied properly, but I'm fairly certain that the `ds_read` instruction was failing the following check within CB last week. So there may have been a class of instructions that weren't having their TSFlags copied?

I don't think you can copy the TSFlags all at once. The ds_read problem should be fixed by the current patch because it sets LGKM_CNT on the Real instruction.

> One function that gets used within the logic is:
>
>   // This is a flat memory operation. Check to see if it has memory tokens other
>   // than LDS. Other address spaces supported by flat memory operations involve
>   // global memory.
>   bool SIInsertWaitcnts::mayAccessVMEMThroughFlat(const MachineInstr &MI) const {
>     assert(TII->isFLAT(MI));
>   
>     // All flat instructions use the VMEM counter.
>     assert(TII->usesVM_CNT(MI));
>   
>     // If there are no memory operands then conservatively assume the flat
>     // operation may access VMEM.
>     if (MI.memoperands_empty())
>       return true;
>   
>     // See if any memory operand specifies an address space that involves VMEM.
>     // Flat operations only supported FLAT, LOCAL (LDS), or address spaces
>     // involving VMEM such as GLOBAL, CONSTANT, PRIVATE (SCRATCH), etc. The REGION
>     // (GDS) address space is not supported by flat operations. Therefore, simply
>     // return true unless only the LDS address space is found.
>     for (const MachineMemOperand *Memop : MI.memoperands()) {
>       unsigned AS = Memop->getAddrSpace();
>       assert(AS != AMDGPUAS::REGION_ADDRESS);
>       if (AS != AMDGPUAS::LOCAL_ADDRESS)
>         return true;
>     }
>   
>     return false;
>   }
>
> (Actually `SIInsertWaitcnts::mayAccessLDSThroughFlat` is another function that is also in the same boat as this one.) I'm not positive if I'll be able to recreate this from within llvm-mca because I'm not sure whether the `MCInst` can be queried for `memoperands`. (It is a `MachineInstr` that is being used in this function, not an `MCInst`.) I don't think there's anything that you can do to correct this though. Maybe the `MCInst` can be queried for `memoperands` and if not, maybe I'll have to make a conservative assumption, but this can be something I explore and discuss further when I submit the patch for AMDGPUCustomBehaviour.

The code above is examining the pointer being loaded from, to see if the compiler statically knows something about which area of memory it points into. There's no way you can do this in MCA for a single load/store instruction in isolation, so I think you just have to "conservatively assume the flat operation may access VMEM".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104293