This does duplicate a lot from hip-config.cmake. I wonder if its possible to reuse that file and maybe have another config variable for language(ie $<COMPILE_LANGUAGE:@LANG@>), then configure it once for HIP and then again for CXX.

This does duplicate a lot from hip-config.cmake. I wonder if its possible to reuse that file and maybe have another config variable for language(ie $<COMPILE_LANGUAGE:@LANG@>), then configure it once for HIP and then again for CXX.

I agree we should find a way to share logic between the two, but I think we should do that after we figure out how to handle the currently sharing of hip::device.

I would prefer to remove this shared dependency as it allows for clear separation of requirements between the HIP language, and users that want to include C++ components of the ROCM toolkit.

I don't know if the better approach is to export hip::device into a separate namespace ( hip-lang::device ) and have the current hip-config also search for hip-lang-config or if we should duplicate the target across both namespaces

Edit:

I will revise the pull request to make hip and hip-lang not share any targets, which is the safest approach as we determine how best to reuse components.

Yes you will, but I don't know what HIP backwards compatibility rules are, and didn't want to presume that I could break existing users that rely on the current flag injection to make C files act as HIP.

I dont think hip::device is used for C language as it requires at least c++11.

I agree we should find a way to share logic between the two, but I think we should do that after we figure out how to handle the currently sharing of hip::device.

Sounds good. There is the flexibility to rename the target.

I don't know if the better approach is to export hip::device into a separate namespace ( hip-lang::device ) and have the current hip-config also search for hip-lang-config or if we should duplicate the target across both namespaces

Is there a way to change the language generator expression from HIP to CXX for hip-lang::device? Or would we make hip-lang::device not restricted to a compile language and then restrict it with target_link_libraries(hip::device INTERFACE $<$<COMPILE_LANGUAGE:CXX>:hip::device>)?

@mangupta @yxsamliu Can you merge this PR?

I dont think hip::device is used for C language as it requires at least c++11.

Currently the rules specified in hip::device via hip-config apply to all source files in a target even if they aren't of the C++ language. For example any C or Fortran would have all the includes, and compile options applied. In the case of C sources this would cause them to be compiled as HIP at best ( due to the -x hip compile option ) to a compile failure when the compiler isn't clang based. Likewise for Fortran sources.

Is there a way to change the language generator expression from HIP to CXX for hip-lang::device?

In short, no :)

The enabling of the HIP language in CMake can't have any side-effects on any other languages. Therefore all rules that constructed by the imported targets must only apply to the HIP language ( We can't even presume CXX is enabled).

Can we make hip-lang::device not restricted to a compile language and then restrict it with target_link_libraries(hip::device INTERFACE $<$<COMPILE_LANGUAGE:CXX>:hip::device>)?

Unforunately this kind of rule doesn't work in CMake. What that currently states is that if they language CXX is enabled link / use hip::device. It doesn't mean that only CXX sources will apply rules/usages associated with hip::device. For include directory, compile option, etc to only apply to a single language they have to be conditionally restricted as I have done in hip-lang-config

I have updated the pull request to have separate targets for hip-lang. In addition I only now generate this file for the rocclr backend since we can't re-use the same targets and I didn't want to spend lots of time fixing the other backends if the design changes due to feedback.

Unforunately this kind of rule doesn't work in CMake.

Ah then we will probably need two different targets and then set them up using different config variables or make a function to set up these targets.

I only now generate this file for the rocclr backend

The other backends are deprecated anyways, so I think this is fine.

@gargrahul Can you merge this?

@robertmaynard Can please rebase this patch against the develop branch instead of the 3.9.x release branch?

@robertmaynard Can please rebase this patch against the develop branch instead of the 3.9.x release branch?

Will do.

Congratulations 🎉. DeepCode analyzed your code in 0.93 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

@mangupta Rebased on top of develop.

@robertmaynard
Can you provide the description of the background and the reason for the PR?

We need the information for internal discussion.

@robertmaynard
Can you provide the description of the background and the reason for the PR?

We need the information for internal discussion.

Here is the reasoning for the new CMake module: #2158

This is missing some of the context of the subtly of finding compilers, CMake compiler verification and what we need to have
things such as try_compile to work correctly. Some of that is discussed in #2154, and some of it is in an email thread that @pfultz2 would be able to provide.

@pfultz2

The other backends are deprecated anyways, so I think this is fine.

How does that relate to the recently released HIP-CPU? Is that an alternative back-end in this sense?

How does that relate to the recently released HIP-CPU? Is that an alternative back-end in this sense?

hip::device and hip::host doesn't make sense for the cpu, nor does treating this a seperate language. Furthermore, hip-cpu uses different cmake targets(such as hip_cpu_rt).

Can you provide the description of the background and the reason for the PR?

Here is a little more background on this change. This document here goes over our current cmake support in HIP.

Currently, our cmake support for hip only supports device compilation if the CXX compiler is set to clang. This is acceptable for our own rocm libraries and many projects as clang is a fully capable C++ compiler.

However, some users want to use a different compiler for host and device compilation, as they may want to pick a compiler that provides better CPU optimizations(such as AOCC or ICC). So Robert is working on adding HIP as a seperate language so a different compiler can be set for CPU and GPU code.

This requires a different cmake target as there are some small difference. First, hip::deviceshould only apply its flags to CXX language(and it will in PR #2183) but to support HIP as a language then the cmake target needs to apply the flags to only HIP and not CXX. Thus we need a different cmake target.

Secondly, for HIP as a language, cmake will add the --offload-arch flags from HIP_ARCHITECTURES in cmake whereas the current hip::device will add the --offload-arch flags based on the GPU_TARGETS cmake variable. Thus the cmake target for HIP language need to skip such flags, however, they are necessary for hip::device for CXX otherwise it will fail compilation.

So users will have two ways to compile hip gpu device code. The first by setting the CXX compiler to our device compiler and then linking with hip::device. The second is to move GPU code to HIP, and set the cmake language to HIP. We need to support both ways. We cant break compatibility with current users and some users may not be able to use the latest cmake version, so they will most likely use hip from CXX.

@robertmaynard Looks like your PR depends on /usr/local/share/cmake-3.19/Modules/CMakeTestHIPCompiler.cmake for HIP Language.
But I cannot find it from the latest cmake 3.19 release (at https://cmake.org/download/)
Is it created by yourself ?

@robertmaynard Looks like your PR depends on /usr/local/share/cmake-3.19/Modules/CMakeTestHIPCompiler.cmake for HIP Language.
But I cannot find it from the latest cmake 3.19 release (at https://cmake.org/download/)
Is it created by yourself ?

The associated changes to CMake aren't part of an offical release. You will need to build the following fork of CMake to test these changes: https://gitlab.kitware.com/robertmaynard/cmake/-/tree/add_hip_language

Note: I force push to this fork at will :)

Thus if the PR is merged, we still need install CMakeTestHIPCompiler.cmake for the PR to work .

Thus if the PR is merged, we still need install CMakeTestHIPCompiler.cmake for the PR to work .

This PR wont break current cmake support. Of course, to use HIP as a seperate compiled language will require a version of cmake that supports it(such as a future version of cmake or robert's fork). CMake will install this file on the versions that support HIP as a lanugage, so I dont see this as being a problem.

Looks good to me but need rebase.
The original codes are too old.

What would you like to me to rebase on? It is currently based on the main branch as of Dec 7th.

Looks good to me but need rebase.
The original codes are too old.

What would you like to me to rebase on? It is currently based on the main branch as of Dec 7th.

Codes on Dec 7th are still not the latest. I don't know when the latest will come to github. Now Hcc runtime and compiler have been removed in the latest codes, so you just need remove Hcc-related codes in the PR.

I think this PR can be merged. I will do some small change in the future.

I think this PR can be merged. I will do some small change in the future.

Going forward, one of the things to look out for is leakage of the hip:: namespace into the generated file. In general this is caused by a target having a PUBLIC linkage to hip::amdhip64 instead of amdhip64. Using amdhip64 allows the correct transformation into hip-lang::amdhip64.

I think this PR can be merged. I will do some small change in the future.

Going forward, one of the things to look out for is leakage of the hip:: namespace into the generated file. In general this is caused by a target having a PUBLIC linkage to hip::amdhip64 instead of amdhip64. Using amdhip64 allows the correct transformation into hip-lang::amdhip64.

This sounds important. We link everywhere to hip::amdhip64. Guidance on compliance needs to be handled.

Guidance on compliance needs to be handled

Currently the only target that uses amdhip64 that is needed by this module is host. So the extent of the requirements is that host should use target_link_libraries(host PUBLIC amdhip64) instead of target_link_libraries(host PUBLIC hip::amdhip64).

Going forward a couple of soultions would be:

• Not share the same host target between two different export sets ( hip, and hip-lang )
• Possibly merge the two export sets together, which has other issues ( the current hip-config C++ behavior would need to be disabled when importing the HIP information )

Could anybody in KitWare add copyright header in hip-lang-config.cmake.in ?
Without copyright header , it's illegal to merge this patch .

Could anybody in KitWare add copyright header in hip-lang-config.cmake.in ?
Without copyright header , it's illegal to merge this patch .

@TomSang what copyright do you need? I looked at the other cmake files and did not see any copyright information. Can you point to a sample cmake file with the required copyright? Thanks.