[PATCH] D129075: [AMDGPU] gfx11 allow dlc for MUBUF atomics

[Dmitry Preobrazhensky via Phabricator via llvm-commits]

dp added a comment.

Nice refactoring!



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:4489
+    S = SMLoc::getFromPointer(&CStr.data()[CStr.find("dlc")]);
+    Error(S, "gfx10 mubuf atomics ignore dlc");
+    return false;
----------------
I believe it would be clearer to handle `dlc` on GFX10 as //unsupported// rather than //ignored// because the patch actually disables this modifier.


================
Comment at: llvm/lib/Target/AMDGPU/BUFInstructions.td:2074
+  MUBUF_Real_gfx11<op, ps, real_name> {
+  let Inst{13}    = cpol{CPolBit.DLC};
+}
----------------
Maybe remove extra spaces?
Maybe rename this class because a multiclass with the same name is defined below.


================
Comment at: llvm/lib/Target/AMDGPU/BUFInstructions.td:2138
+                                                string real_name> {
+  defvar IsRtn = !if(!eq(is_return, 1), "_RTN", "");
+  def _BOTHEN#IsRtn#_gfx11 :
----------------
`Rtn` may be better because `IsRtn' assumes a flag.


================
Comment at: llvm/lib/Target/AMDGPU/MCTargetDesc/AMDGPUInstPrinter.cpp:216
+        (TSFlags & SIInstrFlags::MUBUF) && AMDGPU::isGFX10(STI))
+      O << " /* dlc is ignored for gfx10 mubuf atomics */";
+    else
----------------
I believe the code with `dlc=1` is legal for GFX10, and because `dlc` is ignored, I think we should silently drop it instead of emitting a warning.

It would be nice to have a test for this case.


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

https://reviews.llvm.org/D129075