sameerds added a comment.
In D121951#3411856 <https://reviews.llvm.org/D121951#3411856>, @scott.linder
wrote:
> @yaxunl Does excluding device-libs via COV_None make sense?
>
> @sameerds Would you still rather we just not add this attribute in the
> frontend at all? I'm OK with it if that is the consensus
Yes, I still think there is no need to emit that attribute in the frontend. It
will always be inferred by the Attributor when optimization is enabled. This
also eliminates the check for COV_None and there seems to be some uncertainty
about COV_None anyway. This also eliminates the updates to all the tests where
the no-hostcall-ptr attribute does not actually matter. If ever we need to
check if hostcall is being used on OpenCL and COV < 5, we should do it per
feature and inform the user appropriately.
================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUHSAMetadataStreamer.cpp:406-408
if (Func.getParent()->getNamedMetadata("llvm.printf.fmts"))
emitKernelArg(DL, Int8PtrTy, Align(8), ValueKind::HiddenPrintfBuffer);
+ else if (!Func.hasFnAttribute("amdgpu-no-hostcall-ptr"))
----------------
scott.linder wrote:
> sameerds wrote:
> > I would structure this differently: If this is code-object-v4 or lower,
> > then if "llvm.printf.fmts" is present, then this kernel clearly contains
> > OpenCL bits, and cannot use hostcall. So it's okay to just assume that the
> > no-hostcall-ptr attribute is always present in this situation, which means
> > the only metadata generated is for ValueKind::HiddenPrintfBuffer. Else if
> > this is code-object-v5, then proceed to emit both metadata.
> >
> >
> I'm not sure I follow; doesn't the code as-is implement what you're
> describing?
>
> If the printf metadata is present, this will only ever emit the
> `HiddenPrintfBuffer`, irrespective of the hostcall attribute. Otherwise, this
> respects `amdgpu-no-hostcall-ptr` (i.e. for HIP and other languages).
>
> The "if this is code-object-v4 or lower" portion is implicit, as this is just
> the `MetadataStreamerV2` impl. The `MetadataStreamerV3` (below) and
> `MetadataStreamerV4` (inherits from V3) impls below are similar. The
> `MetadataStreamerV5` impl is already correct for V5 (i.e. supports both
> argument kinds for the same kernel).
Your last para about different streamers cleared the confusion. So, this looks
good.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121951/new/
https://reviews.llvm.org/D121951
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
