[PATCH] D127315: [AMDGPU] New GFX11 intrinsic llvm.amdgcn.s.sendmsg.rtn

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:213
+def int_amdgcn_s_sendmsg_rtn : Intrinsic <[llvm_anyint_ty], [llvm_i32_ty],
+  [ImmArg<ArgIndex<0>>, IntrNoMem, IntrHasSideEffects]>;
+
----------------
arsenm wrote:
> Missing a lot of the default attributes (which all of the other intrinsics are also currently missing)
Is that an objection? :)


================
Comment at: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.sendmsg.rtn.ll:127
+}
+
+declare i32 @llvm.amdgcn.s.sendmsg.rtn.i32(i32)
----------------
arsenm wrote:
> Maybe test some invalid immediates?
Done, but validity is a slippery concept here. Do you think these should be diagnosed at some stage? I would be tempted to say the intrinsic should at least accept any unsigned 8-bit immediate, which is what the instruction encodes.

(The next level of verification would be to insist that bit 7 is set, since that distinguishes messages that are expected to return something. And the next level after that would be to only accept the specific message numbers that are currently defined to do something useful.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127315