[PATCH] D70038: [AMDGPU] add support for hostcall buffer pointer as hidden kernel argument

[Matt Arsenault via Phabricator via llvm-commits]

arsenm accepted this revision.
arsenm added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:427
+      if (HostcallUsed) {
+        OutContext.reportError({}, "Cannot use both hostcall and printf.");
+        return;
----------------
sameerds wrote:
> arsenm wrote:
> > sameerds wrote:
> > > arsenm wrote:
> > > > This should probably be using the LLVMContext error, instead of the MCContext error to at least report the function context in the error, e.g:
> > > > 
> > > > 
> > > > ```
> > > >     DiagnosticInfoUnsupported NoGraphicsHSA(
> > > >         Fn, "unsupported non-compute shaders with HSA", DL.getDebugLoc());
> > > >     DAG.getContext()->diagnose(NoGraphicsHSA);
> > > > ```
> > > > 
> > > > There should also be a testcase hitting this error
> > > Moved this whole check to the printf runtime binding pass. This is more appropriate since the check is happening where the printf feature is actually processed.
> > > 
> > > There already was a test for the error check, replaced it with a newer test.
> > Asserting here instead of the error is worse from a MIR testing perspective
> Why is MIR testing relevant here? printf and hostcall are high-level language features that we are grudgingly handling in the backend for now. Using them both in the same low-level module is undefined, and there is no reason to even write an MIR based test for them.
> 
> Also, if asserting is bad, are you asking to keep the error or only remove the assert?
Because the property enforced by this pass might not be enforced in a MIR run. Asserting on a not-bug edge case MIR sometimes wastes time debugging


================
Comment at: llvm/test/CodeGen/AMDGPU/opencl-printf-no-hostcall.ll:16
+
+declare <2 x i64> @__ockl_hostcall_internal(i8* %0, i32 %1, i64 %2, i64 %3, i64 %4, i64 %5, i64 %6, i64 %7, i64 %8, i64 %9)
+
----------------
Drop the argument names here which I'm surprised even parse?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70038