[PATCH] D70038: [AMDGPU] add support for hostcall buffer pointer as hidden kernel argument
Matt Arsenault via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 19 04:40:00 PST 2019
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
More information about the llvm-commits
mailing list