[PATCH] D138216: [AMDGPU] Intrinsic to expose s_wait_event for export ready

[David Stuttard via Phabricator via llvm-commits]

dstuttard added inline comments.


================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:2070
 
+def int_amdgcn_wait_event_export_ready :
+  Intrinsic<[], [], [IntrNoMem, IntrHasSideEffects, IntrWillReturn]
----------------
foad wrote:
> Should this be int_amdgcn_s_wait_event_export_ready? I'm not sure how consistent we are about including the "s" or "v" prefix in intrinsic names.
I'm in two minds about this since the intrinsic doesn't strictly map to the instruction. I think I slightly prefer this one.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:8375
                                       Op->getOperand(2), Chain), 0);
+  case Intrinsic::amdgcn_wait_event_export_ready:
+    return SDValue(DAG.getMachineNode(AMDGPU::S_WAIT_EVENT, DL, MVT::Other,
----------------
foad wrote:
> Why do you need this? Can't you do it with a tablegen selection pattern?
Yes, I'll update it. There are potentially some complexities that may have to be handled in a later patch, but I think they can also be handled in tablegen too.


================
Comment at: llvm/test/CodeGen/AMDGPU/llvm.amdgcn.wait.event.ll:1
+; RUN: llc -march=amdgcn -verify-machineinstrs -mcpu=gfx1100 < %s | FileCheck -check-prefix=GCN %s
+
----------------
foad wrote:
> Should test globalisel as well.
Yes, will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138216