LGTM.

In D146686#4215577, @MaskRay wrote:

This change is correct for Linux. llvm/CMakeLists.txt says:

if(CMAKE_SYSTEM_NAME MATCHES "BSD|Linux|OS390")
  set(LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default ON)

Some rpath using OSes (notably macOS) use LLVM_ENABLE_PER_TARGET_RUNTIME_DIR_default=OFF. Is the rpath setting ever usable on macOS?
If not, the change is correct.

LGTM. Thanks!

Do you know whether the issue happens to HIP too? Thanks.

ping

fix tests

In D145721#4182834, @tra wrote:

clang should pass -mno-amdgpu-ieee to -cc1

It would be useful to have some details on why we should pass that option.

In D140663#4203604, @tra wrote:

It appears that this patch may be causing a use-after free when we attempt to generate kernel registration code.
The root cause is that the value we insert into KernelHandles by name is later on replaced by a different instance of the global value with the same name.
AFAICT, the invalidation issue was present before but we accidentally avoided it because we only looked up the still-valid new entries. The dangling references were still in the map, but not accessed.

LGTM. Please update the description.

fix tests

LGTM on clang side.

In D145770#4185658, @lamb-j wrote:

In D145770#4184996, @yaxunl wrote:

The description needs fix. "ABI field" should be "environment component".

Also, we need a clang-offload-bundler test which bundles with a non-canonical triple and unbundles with a canonical triple and vice versa.

I'm not sure if we can use Triple.normalize(). Here's what I got from a small test:

amdgcn-amd-amdhsa  ->  amdgcn-amd-amdhsa (no env field added)
amdgcn-amd-amdhsa- ->  amdgcn-amd-amdhsa-unknown (empty env changed to unknown)

Also are we sure ABI is incorrect? That's what is used here:

https://clang.llvm.org/docs/CrossCompilation.html

(I'm happy to change to environment, just double checking)

The description needs fix. "ABI field" should be "environment component".

HIP toolchain does not pass -fopenmp-targets=amdgcn-amd-amdhsa to clang -cc1 in host compilation. It does not pass -fopenmp-is-device to clang -cc1 in device compilation. Without these options clang will not generate OpenMP offloading code for amdgpu in device and host compilation. The host code should still function correctly.

In D145591#4182788, @jhuber6 wrote:

In D145591#4182748, @yaxunl wrote:

In D145591#4182360, @jhuber6 wrote:

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

We cannot filter out -fopenmp for HIP job because the host code in HIP program needs it to support "omp parallel for" etc. Filtering it will break existing HIP programs.

I mean, couldn't we just prevent the -cc1 arguments for the HIP device compilation from using any OpenMP? Or is that breaking. I figured it was only supported for the CPU portion.

In D145591#4182360, @jhuber6 wrote:

I'm not a fan of the same warning being copied in 24 places. Why do we set LangOpts.IsOpenMP on the GPU compilation side, couldn't we just filter out the -fopenmp or whatever it is for the HIP job?

In D145591#4182351, @tra wrote:

In D145591#4182168, @yaxunl wrote:

-x hip and -fopenmp has been a valid combination. -fopenmp with -x hip allows non-offloading OpenMP directives in host code in HIP. It just ignores the offloading directives.

That brings me back to the earlier question -- what do we currently do when target directives are encountered when we compile a C++ source w/ OpenMP enabled and why HIP shold be handled differently.

If a warning makes sense for target directives with offloading disabled, that warning would be equally applicable to C/C++/CUDA & HIP. If that's not the case, what am I missing?

In D145393#4172429, @MaskRay wrote:

Seems fine. Should we eventually remove --offload-add-rpath and -fopenmp-implicit-rpath?

fix tests

In D145591#4182016, @tra wrote:

It sounds like what we want is to make -x hip and -fopenmp mutually exclusive, with a hard error when both are used. If you look at the problem as "-fopenmp completely breaks HIP compilation", a warning is a bit too weak of a measure, IMO.

We can restate it all as "mixing offloading modes is not supported" and generalize it to both CUDA and HIP.

-x hip and -fopenmp has been a valid combination. -fopenmp with -x hip allows non-offloading OpenMP directives in host code in HIP. It just ignores the offloading directives.

fix comments and tests

LGTM. Thanks.

Did you go through the deprecation process for this option? We need to send a documentation team first so that it is announced to the users. Then in the next release we can deprecate it.

revised by Artem's comments

In D145509#4175773, @tra wrote:

LGTM, but we should probably get someone familiar with macos to chime in, just in case there may be some reason behind macos using temp directories here.

This change is OK for MacOS as lipo does not requires specific

I'm curious why lipo has been singled out. Is that the only use case that ends up using this path?

In D145343#4170250, @arsenm wrote:

I think exposing whether or not the flag was used is weird/broken, as is including _OPTION in the name. Should just define to whether it's enabled or not

LGTM. Thanks

revised by Fanrui's comments

In D144011#4126853, @MaskRay wrote:

I think it makes sense for -Wsign-conversion to not warn for this LP64 case, like we don't emit a warning for -m32. I do not know whether we need another diagnostic like -Wshorten-64-to-32 for this case, but am inclined to no.

I wonder whether the newly added condition can be merged with the following condition:

if ((!isa<EnumType>(Target) || !isa<EnumType>(Source)) &&
    ((TargetRange.NonNegative && !LikelySourceRange.NonNegative) ||
     (!TargetRange.NonNegative && LikelySourceRange.NonNegative &&
      LikelySourceRange.Width == TargetRange.Width))) {
  if (S.SourceMgr.isInSystemMacro(CC))

In D144047#4129182, @shangwuyao wrote:

In D144047#4129154, @yaxunl wrote:

Making the builtin types consistent is necessary to keep struct layout consistent across host and device, but why do we need to make __GCC_ATOMIC_XXX_LOCK_FREE macros the same between the host and device? Is there any concrete issue if they are not the same?

The reason is the same as NVPTX, see https://github.com/llvm/llvm-project/blob/22882c39df71397cc6f9774d18e87d06e016c55f/clang/lib/Basic/Targets/NVPTX.cpp#L137-L141. Without it, we won't be able to use libraries that statically check the __atomic_always_lock_free. I could add the comments in the code if that makes things more clear.

Making the builtin types consistent is necessary to keep struct layout consistent across host and device, but why do we need to make __GCC_ATOMIC_XXX_LOCK_FREE macros the same between the host and device? Is there any concrete issue if they are not the same?

In D144011#4126553, @shafik wrote:

If I look at the clang docs for Wconversion I see it includes -Wshorten-64-to-32 which I believe this is a case of. I think maybe the warning needs a better warning for this case?

In D143495#4123119, @pvellien wrote:

@yaxunl Could you please commit this change on my behalf? I don't have a write access to the trunk.
Thank you

LGTM. Thanks

LGTM. Thanks.

LGTM. Thanks

LGTM. Thanks

LGTM. Thanks

LGTM. Thanks.

LGTM. Thanks.

LGTM. Thanks.

@scchan the test expects CLANG_INSTALL_LIBDIR_BASENAME to be lib. Maybe in the failed test it was not true.

Can we keep the original behaviour for the old driver for HIP? Only enable the change for the new driver.

Is this patch really necessary? -mcode-object-version= option does not go through any translation in derived args. If it is in derived args, it should also be in the original args. HIPAMD toolchain just uses the original driver args. Why OpenMP toolchain needs to use the derived args?