The title says --offline option, which should be --offload.

I am OK for now. We may need keep an eye for the perf regression and be prepared to figure a way to alleviate that.

LGTM. Thanks.

This may cause some perf degradation.

In D115523#3237857, @Anastasia wrote:

In D115523#3237410, @yaxunl wrote:

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

Potentially we should append the name of the translation unit to all kernel wrapper names for the enqueued blocks to resolve this? For example, global constructors stubs are using such a similar naming scheme taken from the translation unit.

But the kernel function in OpenCL has to be globally visible and many tools have been built with this assumption. Additionally, some toolchains might require the enqueued kernels to be globally visible as well in order to access them as an execution entry point.

It is possible that block kernels are defined and invoked in static functions, therefore two block kernels in different TU's may have the same name. Making such kernels external may cause duplicate symbols.

avoid clearing AL

I think it will be cleaner to keep the original amdgpu-asan.cu unchanged whereas add amdgpu-asan-printf.cu which tests asan with printf.

In D116216#3209335, @pvellien wrote:

@yaxunl It would be very much helpful to know how to write test coverage for this particular patch? thanks

LGTM. Thanks.

In D110622#3199233, @linjamaki wrote:

Assuming that this patch is ready to land. @tra or @yaxunl, could you please commit this patch to the LLVM for us? Thanks.

In D115661#3193413, @yaxunl wrote:

In D115661#3193157, @arsenm wrote:

In D115661#3193152, @yaxunl wrote:

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

The two lit tests which changed from addr space 4 to 1 demonstrated that. In alias analysis, if a variable is in addr space 4, the backend knows that it is constant and can do optimizations on it. After changing to addr space 1, those optimizations are gone.

The backend also knows because the constant flag is set on the global variable. Addrspace(4) is a kludge which is largely redundant with other mechanisms for indicating constants

If backend can only rely on constant flag then we do not need put global variables in constant addr space.

Let's leave this patch as it is now. And revisit it if there are any regressions found.

In D115661#3193157, @arsenm wrote:

In D115661#3193152, @yaxunl wrote:

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

The two lit tests which changed from addr space 4 to 1 demonstrated that. In alias analysis, if a variable is in addr space 4, the backend knows that it is constant and can do optimizations on it. After changing to addr space 1, those optimizations are gone.

The backend also knows because the constant flag is set on the global variable. Addrspace(4) is a kludge which is largely redundant with other mechanisms for indicating constants

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

This may cause perf regressions for HIP.

In D110549#3185409, @linjamaki wrote:

Assuming this patch is ready to land. @yaxunl, Could you please commit this patch to the LLVM for us. Thanks.

In D115283#3183034, @JonChesterfield wrote:

In D115283#3182879, @yaxunl wrote:

In D115283#3181128, @JonChesterfield wrote:

Not exactly that. The weak symbol isn't the function name, as that gets renamed or inlined.

We discussed this before. As code object ABI use runtime metadata to represent hostcall_buffer, we need to check whether hostcall is needed by IR.

This approach will require checking asm instructions inside a function to determine whether this function requires hostcall. It is hacky for IR representation.

There are two approaches here:
1/ Tag the function using inline asm and totally ignore it in the compiler. HSA/etc tests per-code-object if the symbol is present
2/ Tag the function (in source or in compiler), propagate information to llc, embed it in msgpack data, HSA/etc tests per-function if the field is present

2/ is somewhat useful if we elide the 8 byte slot of kernarg memory for functions that don't use it, otherwise it just increases work done by the runtime. Instead of checking for presence of one symbol (a hashtable lookup), it's a linear scan through msgpack data. We don't currently elide those 8 bytes, so right now this is making the compiler more complicated in exchange for making the runtime slower.

1/ has the benefit of being dead simple and totally compiler agnostic, and the cost of passing the 8 byte hostcall thing to every function in a code object that asked for it.

In D115283#3181109, @kpyzhov wrote:

In D115283#3180836, @yaxunl wrote:

If we only need to check whether __ockl_hostcall_internal exists in the final module in LLVM codegen to determine whether we need the hostcall metadata, probably we don't even need a function attribute or even module flag.

Right, we used to do exactly that (just check at the CodeGen phase if 'ockl_hostcall_internal()' is present in the module), but then it turned out that it does not work with -fgpu-rdc since IPO may rename the 'ockl_hostcall_internal()'.

In D115283#3181128, @JonChesterfield wrote:

Not exactly that. The weak symbol isn't the function name, as that gets renamed or inlined.

If we only need to check whether __ockl_hostcall_internal exists in the final module in LLVM codegen to determine whether we need the hostcall metadata, probably we don't even need a function attribute or even module flag.

LGTM. Thanks.

One drawback of this approach is that it does not work for LLVM modules generated from assembly or programmatically e.g. Tensorflow XLA.

In D110622#3177111, @tra wrote:

In D110622#3176804, @yaxunl wrote:

So, the question is -- what's the right way to specify something like this in a consistent manner?
--offload option proposed here does not seem to be a good fit. It was intended as a more flexible way to create a single -cc1 sub-compilation and we're doing quite a bit more here.

Does --offload-arch=spirv* fit better here? If I understand the goal of this patch correctly, it tries to provide controls for changing offload target for HIP application from default (AMDGCN) to SPIR-V.

--offload-arch= only accepts GPU arch which is translated to processor option (-mcpu= or -march=) in clang -cc1. spirv is a target triple which is not suitable for --offload-arch=.

--offload= is supposed to cover both target triple and processor with some flexibility. If only target triple is specified, it assumes default processor. If only processor is specified, it deduces target triple. It also allows both triple and processor. In this case, --offload=spirv translates to -triple spirv -mcpu=generic.

So, one would expect that we should be able to specify it more than once to target multiple GPU variants, if we were to use it as a more flexible --offload-arch.
If I read the tests correctly, using --offload= limits us to exactly one variant now. Perhaps it should eventually be relaxed to only enforce single --offload= variant if we're offloading to SPIR-V. It's not a showstopper for this patch. We can relax it later.

So, the question is -- what's the right way to specify something like this in a consistent manner?
--offload option proposed here does not seem to be a good fit. It was intended as a more flexible way to create a single -cc1 sub-compilation and we're doing quite a bit more here.

Does --offload-arch=spirv* fit better here? If I understand the goal of this patch correctly, it tries to provide controls for changing offload target for HIP application from default (AMDGCN) to SPIR-V.

In D115039#3174154, @tra wrote:

Put __hip_gpubin_handle in comdat when it has linkonce_odr linkage.

I wonder when would this happen? I'm not sure we ever want gpubin handles from different TUs merged. I think it may result in different TUs attempting to load/init the same GPU binary multiple times.

Ideally, we could let the builtins accept both vec3 and vec4. But I am OK with this for now. I think the overhead may be minimal.

In D114957#3167703, @arsenm wrote:

In D114957#3167700, @arsenm wrote:

I think this macro is purely terrible and should not be added (and at least should be all caps?). If we can't just hard break users, I would rather just leave the builtin signatures broken

Rather than adding an ad-hoc named macro, could they just directly check the clang version?

LGTM from clang side. Thanks.

In D114957#3166936, @foad wrote:

In D114957#3166858, @yaxunl wrote:

In D114957#3166817, @foad wrote:

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

This breaks users' code. If we have to do this, at least let clang emit a pre-defined macro e.g. __amdgcn_bvh_use_vec3__=1 so that users can make their code work before and after the change.

I don't know anything about OpenCL macros. Is it good enough to put this in AMDGPUTargetInfo::getTargetDefines:

if (Opts.OpenCL)
  Builder.defineMacro("__amdgcn_bvh_use_vec3__");

Does it need tests, documentation, etc?

In D114957#3166974, @foad wrote:

In D114957#3166948, @b-sumner wrote:

In D114957#3166936, @foad wrote:

In D114957#3166858, @yaxunl wrote:

In D114957#3166817, @foad wrote:

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

This breaks users' code. If we have to do this, at least let clang emit a pre-defined macro e.g. __amdgcn_bvh_use_vec3__=1 so that users can make their code work before and after the change.

I don't know anything about OpenCL macros. Is it good enough to put this in AMDGPUTargetInfo::getTargetDefines:

if (Opts.OpenCL)
  Builder.defineMacro("__amdgcn_bvh_use_vec3__");

Does it need tests, documentation, etc?

But how long would that be carried? And then deprecated?

Then do you think the patch is OK as-is?

In D114957#3166861, @arsenm wrote:

In D114957#3166858, @yaxunl wrote:

In D114957#3166817, @foad wrote:

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

This breaks users' code. If we have to do this, at least let clang emit a pre-defined macro e.g. __amdgcn_bvh_use_vec3__=1 so that users can make their code work before and after the change.

I do not think it's worth introducing a macro for this. Are there actually C users of these builtins?

In D114957#3166817, @foad wrote:

This is a flag-day change to the signatures of the LLVM intrinsics and the OpenCL builtins. Is that OK?

LGTM. Thanks.

I am not sure whether we want to define a similar macro for cuda-clang.

LGTM. Thanks.

we also need a sema test like clang/test/SemaOpenCL/atomic-ops.cl

could you please include a complete diff context in the patch? You can do that by using git diff -U9999

In D113925#3141011, @gandhi21299 wrote:

@yaxunl thanks for the review! My Github account is locked unfortunately so I will have to ask you to push this commit to the main branch. Thank you!

LGTM. Thanks.

LGTM. I will defer to @tra

LGTM. I will leave to @tra about -nohipwrapperinc

LGTM. Thanks.

Hi Michael, would you like to continue working on this, or let someone from AMD to take over? Thanks.

Revised by Evgenii's comments

I did an experiment regarding the ICF issue and it seems not to affect kernel stub.

@eugenis Any further changes needed? Thanks.

LGTM. Thanks.

I think probably it is necessary to merge linkonce_odr symbols for them to work properly.

LGTM. It seems all concerns have been addressed. Shall we move ahead and land this patch? Thanks.