[PATCH] D146701: [AMDGPU] Create Subtarget Features for some of 16 bits atomic fadd instructions

Thu Mar 23 09:48:47 PDT 2023

foad added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/BUFInstructions.td:2891

+let SubtargetPredicate = HasAtomicFaddNoRtnInsts in {
+defm BUFFER_ATOMIC_ADD_F32    : MUBUF_Real_Atomic_vi <0x4d>;
----------------
mariusz-sikora-at-amd wrote:
> foad wrote:
> > Is this still required?
> No. We can remove this. But I wanted to limit this change only to atomic f16/bf16 and not going deeper 
OK. I thought this was something you added in this patch, but now I see it is just moved around.

================
Comment at: llvm/lib/Target/AMDGPU/FLATInstructions.td:1915

+defm GLOBAL_ATOMIC_PK_ADD_F16 : FLAT_Global_Real_Atomics_vi <0x04e, 0>;
+defm GLOBAL_ATOMIC_PK_ADD_BF16 : FLAT_Global_Real_Atomics_vi<0x52>;
----------------
mariusz-sikora-at-amd wrote:
> foad wrote:
> > Are these changes (from here to the end of the file) still required?
> Not sure if I understand what you mean. Could you please elaborate more ? Are you referring to the fact that both flat_atomic and global_atomic have FLAT encoding and could be unified ?
> I thought this is required, but now you got me thinking ...
I don't understand why the changes from here to the end of the file are required. It looks like you have just moved some definitions around, so that they no longer have a SubtargetPredicate applied. Is that correct? Why?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146701