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

[Sameer Sahasrabuddhe via Phabricator via llvm-commits]

sameerds marked an inline comment as done.
sameerds added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:427
+      if (HostcallUsed) {
+        OutContext.reportError({}, "Cannot use both hostcall and printf.");
+        return;
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70038