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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:587
+    for (auto &U : HostcallFunction->uses()) {
+      if (auto *CI = dyn_cast<CallInst>(U.getUser())) {
+        M.getContext().emitError(CI, "Cannot use both printf and hostcall in the same module");
----------------
Should use CallBase in case an invoke is is ever used


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUPrintfRuntimeBinding.cpp:588
+      if (auto *CI = dyn_cast<CallInst>(U.getUser())) {
+        M.getContext().emitError(CI, "Cannot use both printf and hostcall in the same module");
+      }
----------------
I think this goes over 80?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70038