Closed saiislam closed 5 months ago
@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-backend-amdgpu
Author: Saiyedul Islam (saiislam)
@llvm/pr-subscribers-clang
Author: Saiyedul Islam (saiislam)
It will fix tests like: targetid_multi_image
:warning: C/C++ code formatter, clang-format found issues in your code. :warning:
I don't think we want to support this. -march
was the wrong option to use in the first place, and upstream LLVM never supported specifying multiple device images with -march
so there isn't a legacy argument in trunk. However, AOMP did support this and if it's deemed too disruptive to request users move to --offload-arch=a,b,c
then we can carry that change in the fork.
It will fix tests like: targetid_multi_image
I think the easier way to fix this is to update the Makefile.
I don't think we want to support this.
-march
was the wrong option to use in the first place, and upstream LLVM never supported specifying multiple device images with-march
so there isn't a legacy argument in trunk. However, AOMP did support this and if it's deemed too disruptive to request users move to--offload-arch=a,b,c
then we can carry that change in the fork.It will fix tests like: targetid_multi_image
I think the easier way to fix this is to update the Makefile.
Irrespective of what AOMP does, I think it makes sense to ensure parity between the two ways of specifying architecture. People have been historically using -Xopenmp-target -march
style, and using the same for multiple architectures seems to be the most obvious choice.
Isn't it quite confusing to tell the users that they can use offload-arch
style for single as well as multiple archs, but can use -march
style only for single arch?
I don't think we want to support this.
-march
was the wrong option to use in the first place, and upstream LLVM never supported specifying multiple device images with-march
so there isn't a legacy argument in trunk. However, AOMP did support this and if it's deemed too disruptive to request users move to--offload-arch=a,b,c
then we can carry that change in the fork.It will fix tests like: targetid_multi_image
I think the easier way to fix this is to update the Makefile.
Irrespective of what AOMP does, I think it makes sense to ensure parity between the two ways of specifying architecture. People have been historically using
-Xopenmp-target -march
style, and using the same for multiple architectures seems to be the most obvious choice. Isn't it quite confusing to tell the users that they can useoffload-arch
style for single as well as multiple archs, but can use-march
style only for single arch?
-march
was the wrong option to use for this from the beginning. It's supposed to be an overriding option and it shouldn't be overloaded to mean something different here. In LLVM / trunk we never supported multiple architectures with the -march
option so I don't see any reason to start now. --offload-arch=
is a complete replacement for this behavior and I consider the single -march
option to be legacy. Even within this it's divergent because HIP / OpenCL / AMDGPU use -mcpu
but the OpenMP toolchain ignored that and uses -march=
.
Using --offload-arch=
is a direct replacement for -march
in all use-cases. It's also easier to use and interoperable with CUDA. I would just change the test, you can replace every use of -march
with --offload-arch
and it will work. See the following.
> clang input.c -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa,nvptx64-nvidia-cuda \
-Xopenmp-target=amdgcn-amd-amdhsa --offload-arch=gfx1030 \
-Xopenmp-target=amdgcn-amd-amdhsa --offload-arch=gfx90a \
-Xopenmp-target=nvptx64-nvidia-cuda --offload-arch=sm_89
> llvm-objdump --offloading a.out
a.out: file format elf64-x86-64
OFFLOADING IMAGE [0]:
kind elf
arch sm_89
triple nvptx64-nvidia-cuda
producer openmp
OFFLOADING IMAGE [1]:
kind elf
arch gfx90a
triple amdgcn-amd-amdhsa
producer openmp
OFFLOADING IMAGE [2]:
kind elf
arch gfx1030
triple amdgcn-amd-amdhsa
producer openmp
I don't think we want to support this.
-march
was the wrong option to use in the first place, and upstream LLVM never supported specifying multiple device images with-march
so there isn't a legacy argument in trunk. However, AOMP did support this and if it's deemed too disruptive to request users move to--offload-arch=a,b,c
then we can carry that change in the fork.It will fix tests like: targetid_multi_image
I think the easier way to fix this is to update the Makefile.
Irrespective of what AOMP does, I think it makes sense to ensure parity between the two ways of specifying architecture. People have been historically using
-Xopenmp-target -march
style, and using the same for multiple architectures seems to be the most obvious choice. Isn't it quite confusing to tell the users that they can useoffload-arch
style for single as well as multiple archs, but can use-march
style only for single arch?
-march
was the wrong option to use for this from the beginning. It's supposed to be an overriding option and it shouldn't be overloaded to mean something different here. In LLVM / trunk we never supported multiple architectures with the-march
option so I don't see any reason to start now.--offload-arch=
is a complete replacement for this behavior and I consider the single-march
option to be legacy. Even within this it's divergent because HIP / OpenCL / AMDGPU use-mcpu
but the OpenMP toolchain ignored that and uses-march=
.Using
--offload-arch=
is a direct replacement for-march
in all use-cases. It's also easier to use and interoperable with CUDA. I would just change the test, you can replace every use of-march
with--offload-arch
and it will work. See the following.> clang input.c -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa,nvptx64-nvidia-cuda \ -Xopenmp-target=amdgcn-amd-amdhsa --offload-arch=gfx1030 \ -Xopenmp-target=amdgcn-amd-amdhsa --offload-arch=gfx90a \ -Xopenmp-target=nvptx64-nvidia-cuda --offload-arch=sm_89 > llvm-objdump --offloading a.out a.out: file format elf64-x86-64 OFFLOADING IMAGE [0]: kind elf arch sm_89 triple nvptx64-nvidia-cuda producer openmp OFFLOADING IMAGE [1]: kind elf arch gfx90a triple amdgcn-amd-amdhsa producer openmp OFFLOADING IMAGE [2]: kind elf arch gfx1030 triple amdgcn-amd-amdhsa producer openmp
If -march
is the wrong option then let's start deprecating it and remove it altogether in the next llvm release. But, as long as it is here, it should be equivalent to --offload-arch
.
If
-march
is the wrong option then let's start deprecating it and remove it altogether in the next llvm release. But, as long as it is here, it should be equivalent to--offload-arch
.
Honestly not a bad idea. I could make a patch warning users to use --offload-arch
instead for now.
If
-march
is the wrong option then let's start deprecating it and remove it altogether in the next llvm release. But, as long as it is here, it should be equivalent to--offload-arch
.Honestly not a bad idea. I could make a patch warning users to use
--offload-arch
instead for now.
Sure, let's do that. But, let this land as long as this option is supported.
If
-march
is the wrong option then let's start deprecating it and remove it altogether in the next llvm release. But, as long as it is here, it should be equivalent to--offload-arch
.Honestly not a bad idea. I could make a patch warning users to use
--offload-arch
instead for now.Sure, let's do that. But, let this land as long as this option is supported.
That doesn't track, LLVM has never supported -march
to support multiple options and there's no reason to add it now when we're talking about deprecating it.
If
-march
is the wrong option then let's start deprecating it and remove it altogether in the next llvm release. But, as long as it is here, it should be equivalent to--offload-arch
.Honestly not a bad idea. I could make a patch warning users to use
--offload-arch
instead for now.Sure, let's do that. But, let this land as long as this option is supported.
That doesn't track, LLVM has never supported
-march
to support multiple options and there's no reason to add it now when we're talking about deprecating it.
+1
Thanks for the review and comments. Closing the PR.
Legacy toolchain to handle multiple target architectures specified using
-Xopenmp-target=<triple> -march=<arch>
was only processing a single architecture. This patch also fixes the use of comma to specify multiple archs for a single triple.