scott.linder added inline comments.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9381
+ M.getTarget().getTargetOpts().CodeObjectVersion != 500) {
+ F->addFnAttr("amdgpu-no-hostcall-ptr");
+ }
----------------
arsenm wrote:
> sameerds wrote:
> > The frontend does not need to worry about this attribute. See the comment
> > in the MetadataStreamer. A worthwhile check would be to generate an error
> > if we are able to detect that some hostcall service is being used in OpenCL
> > on code-object-v4 or lower. None exists right now, but we should add the
> > check if such services show up. But those checks are likely to be in a
> > different place. For example, enabling asan on OpenCL for code-object-v4
> > should result in an error in the place where asan commandline options are
> > parsed.
> Should be all opencl, not just kernels. Also < instead of !=?
> The frontend does not need to worry about this attribute. See the comment in
> the MetadataStreamer.
The reason I went this route is that otherwise the MetadataStreamer can only go
off of the presence of the OCL printf metadata to infer that hostcall argument
metadata is invalid and will be ignored by the runtime. Ideally, the
MetadataStreamer or Verifier or something would actually //require// that a
module does not have both OCL printf metadata and functions which are not
"amdgpu-no-hostcall-ptr", but I didn't go that far as it would break old
IR/bitcode that relies on the implicit behavior.
I'm OK with removing this code, and the rest of the patch remains essentially
unchanged. We will still have an implicit rule based on code-object-version and
presence of printf metadata, and at least conceptually you will still be able
to compile OCL for pre-V5 and end up with hostcall argument metadata. That case
is benign if the runtime just ignores it, but it still seems needlessly relaxed
when we can just add the correct attribute in Clang codegen.
> Should be all opencl, not just kernels. Also < instead of !=?
Yes, my mistake, I'll update this
================
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"))
----------------
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).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121951/new/
https://reviews.llvm.org/D121951
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
