This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Support target triple OS component cuda
AbandonedPublic

Authored by yaxunl on Jan 30 2018, 2:48 PM.

Details

Summary

This patch allows AMDGPU backend consumes IR generated by clang for CUDA.

Patch by Greg Rodgers.

Diff Detail

Event Timeline

yaxunl created this revision.Jan 30 2018, 2:48 PM
arsenm requested changes to this revision.Jan 30 2018, 2:55 PM

You're using this just as an alias for AMDHSAOS. We shouldn't add something that behaves exactly the same

lib/Target/AMDGPU/SIISelLowering.cpp
5797

Dead code

This revision now requires changes to proceed.Jan 30 2018, 2:55 PM

You're using this just as an alias for AMDHSAOS. We shouldn't add something that behaves exactly the same

I agree. The IR should just get the right OS to start with.

I think the purpose of this patch is to get a similar usage of clang as nvptx when compiling CUDA, i.e., using cuda as OS instead of using amdhsa as OS and amdgiz as environment. This is more convenient for CUDA application developers since they just need to swap nvptx with amdgcn.

As I understand it, the option users pass to clang++ is --cuda-gpu-arch=<Arch>. Can't we arrange to generate the right triple if they use gfx900 or some other AMD target name for <Arch>?

I think the purpose of this patch is to get a similar usage of clang as nvptx when compiling CUDA, i.e., using cuda as OS instead of using amdhsa as OS and amdgiz as environment. This is more convenient for CUDA application developers since they just need to swap nvptx with amdgcn.

This is a frontend driver question at most. The backend shouldn't need to be aware of this

I think the purpose of this patch is to get a similar usage of clang as nvptx when compiling CUDA, i.e., using cuda as OS instead of using amdhsa as OS and amdgiz as environment. This is more convenient for CUDA application developers since they just need to swap nvptx with amdgcn.

This is a frontend driver question at most. The backend shouldn't need to be aware of this

There are various places in clang where selection is done based on OS==CUDA. If we don't use that OS, we need more complex logic in clang for such choices. I can try making changes to clang to make it work, but I suspect there may be places using OS==CUDA is necessary since it may be needed before parsing the language options.

I think the purpose of this patch is to get a similar usage of clang as nvptx when compiling CUDA, i.e., using cuda as OS instead of using amdhsa as OS and amdgiz as environment. This is more convenient for CUDA application developers since they just need to swap nvptx with amdgcn.

This is a frontend driver question at most. The backend shouldn't need to be aware of this

There are various places in clang where selection is done based on OS==CUDA. If we don't use that OS, we need more complex logic in clang for such choices. I can try making changes to clang to make it work, but I suspect there may be places using OS==CUDA is necessary since it may be needed before parsing the language options.

Changing those selections make sense to me since was are adding support for a completely new and different target.

jlebar added a subscriber: jlebar.Jan 31 2018, 9:16 AM

Changing those selections make sense to me since was are adding support for a completely new and different target.

As one of the CUDA maintainers, this also makes sense to me, although we're going to need tests and (to the extent possible) wrapper functions so that it doesn't break.

Changing those selections make sense to me since was are adding support for a completely new and different target.

As one of the CUDA maintainers, this also makes sense to me, although we're going to need tests and (to the extent possible) wrapper functions so that it doesn't break.

There is another concern. If we need to identify IR originated from CUDA in AMDHSA OS, what should we do? Introduce CUDA again as an environment component?

Changing those selections make sense to me since was are adding support for a completely new and different target.

As one of the CUDA maintainers, this also makes sense to me, although we're going to need tests and (to the extent possible) wrapper functions so that it doesn't break.

There is another concern. If we need to identify IR originated from CUDA in AMDHSA OS, what should we do? Introduce CUDA again as an environment component?

There shouldn't be any reason to do this. The IR shouldn't really care what the source was.

Changing those selections make sense to me since was are adding support for a completely new and different target.

As one of the CUDA maintainers, this also makes sense to me, although we're going to need tests and (to the extent possible) wrapper functions so that it doesn't break.

There is another concern. If we need to identify IR originated from CUDA in AMDHSA OS, what should we do? Introduce CUDA again as an environment component?

There shouldn't be any reason to do this. The IR shouldn't really care what the source was.

Then why do we need opencl environment component in the triple?

Changing those selections make sense to me since was are adding support for a completely new and different target.

As one of the CUDA maintainers, this also makes sense to me, although we're going to need tests and (to the extent possible) wrapper functions so that it doesn't break.

There is another concern. If we need to identify IR originated from CUDA in AMDHSA OS, what should we do? Introduce CUDA again as an environment component?

There shouldn't be any reason to do this. The IR shouldn't really care what the source was.

Then why do we need opencl environment component in the triple?

This specifies the ABI change for the entry point

mkuron added a subscriber: mkuron.Mar 31 2018, 2:34 AM
arsenm resigned from this revision.Apr 5 2020, 7:38 AM
yaxunl abandoned this revision.Apr 5 2020, 9:31 AM

no longer needed