[PATCH] D125498: [AMDGPU] gfx11 scalar alu instructions

[Joe Nash via Phabricator via llvm-commits]

Joe_Nash marked 3 inline comments as done.
Joe_Nash added a comment.

In D125498#3510172 <https://reviews.llvm.org/D125498#3510172>, @arsenm wrote:

> Missing test for intrinsics selection?

I've removed the intrinsic from this patch and will add it in a later patch with tests.



================
Comment at: llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:6394
+    Value = StringSwitch<int>(ValueName)
+                .Case("SAME", 0)
+                .Case("NEXT", 1)
----------------
rampitec wrote:
> All these magic numbers shall go into SIDefines.h.
Good idea for a refactor, can we do this in a later patch?


================
Comment at: llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:500
+    Res = tryDecodeInst<uint64_t>(DecoderTableGFX1132, MI, DW, Address);
+    if (Res) break;
+
----------------
arsenm wrote:
> Seems weird that we aren’t guarding each table lookup with subtarget checks 
It would be possible, but may be redundant with the more fine grained checks done in decodeInstruction via AMDGPUGenDisassemblerTables.inc::checkDecoderPredicate. In any case, I don't think we want that change for this patch.


================
Comment at: llvm/lib/Target/AMDGPU/SOPInstructions.td:1287
 let Uses = [EXEC, M0] in {
-// FIXME: Should this be mayLoad+mayStore?
 def S_SENDMSG : SOPP_Pseudo <"s_sendmsg" , (ins SendMsgImm:$simm16), "$simm16",
----------------
foad wrote:
> This hunk is nothing to do with GFX11, right? Please commit it separately (consider it pre-approved).
Did you mean just the comment or also including let hasSideEffects = 1? I've created a patch with both, but it appears to be NFC. Should this have a test? https://reviews.llvm.org/D125569


================
Comment at: llvm/test/MC/AMDGPU/gfx11_pretest.s:1
+// RUN: llvm-mc -arch=amdgcn -mcpu=gfx1100 -show-encoding %s | FileCheck --check-prefix=GFX11 %s
+
----------------
rampitec wrote:
> What does 'pretest' mean in the file name?
I shall move the tests to gfx11_asm_scalar.s as a separate file is not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125498