[PATCH] D121951: [AMDGPU][OpenCL] Add "amdgpu-no-hostcall-ptr" in Clang codegen pre-COV_5

[Scott Linder via Phabricator via cfe-commits]

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