[SYCL][ROCm] Use offload-arch instead of mcpu for AMD arch #4239

npmiller · 2021-08-03T14:54:49Z

This patch changes using -mcpu for SYCL applications targeting AMD to
-Xsycl-target-backend --offload-arch.

Before this patch the offloading arch wasn't set correctly for AMD
architectures.

This is fixing an issue with HIP that was talked about in #4133,
regarding having v4 in the hip part of the triple, without the v4
HIP seems to be ignoring the fact that the offloading arch is missing
from the triple, which is why there was a workaround orignally to force
not using v4 with SYCL. By fixing the offloading arch this patch fixes
the issue properly and now the triple with v4 works because it also
contains the offloading architecture.

npmiller · 2021-08-03T14:56:21Z

@malixian This patch should fix the issue discussed in:

LLVM and SPIRV-LLVM-Translator pulldown (WW30) #4133 (comment)

AGindinson

I believe clang/test/Driver/sycl-offload-amdgcn.cpp should be expanded, at least to check the error message and enforce valid command lines for the main test cases.

clang/lib/Driver/Driver.cpp

elizabethandrews

Please add a test for the new diagnostic

clang/include/clang/Basic/DiagnosticDriverKinds.td

This patch changes using `-mcpu` for SYCL applications targeting AMD to `-Xsycl-target-backend --offload-arch`. Before this patch the offloading arch wasn't set correctly for AMD architectures. This is fixing an issue with HIP that was talked about in intel#4133, regarding having `v4` in the hip part of the triple, without the `v4` HIP seems to be ignoring the fact that the offloading arch is missing from the triple, which is why there was a workaround orignally to force not using `v4` with SYCL. By fixing the offloading arch this patch fixes the issue properly and now the triple with `v4` works because it also contains the offloading architecture.

Co-authored-by: Artem Gindinson <artem.gindinson@intel.com>

npmiller · 2021-08-04T14:14:27Z

I've updated clang/test/Driver/sycl-offload-amdgcn.cpp to add the architecture parameters, added a test for the new diagnostic in there as well, and updated other amdgcn tests with architecture flags.

I'm still seeing one test failing with this when running ninja check, Driver/amdgpu-openmp-toolchain.c, however it seems that this test was already failing on AMD before this patch.

malixian · 2021-08-06T02:24:35Z

clang/lib/Driver/Driver.cpp

+        if (Triple.isAMDGCN() && llvm::none_of(GpuArchList, [&](auto &P) {
+              return P.first.isAMDGCN();
+            })) {
+          C.getDriver().Diag(clang::diag::err_drv_sycl_missing_amdgpu_arch);


How about adding a default AMD GPU arch?

The reason I didn't go for the default AMD GPU arch is that as I understand it we need to specify the exact GPU architecture for AMD so a default would only work for a very specific type of GPUs. Which means that in a lot of cases users would still need to specify the architecture manually, so I think it is better to force the architecture to always be set manually and have a clear diagnostic, than have a default architecture that rarely works and a more confusing error message from hip.

This is different with NVidia because SM_50 covers a lot of different GPUs, so in most cases it will work out of the box and the user won't have to set the architecture.

clang/lib/Driver/Driver.cpp

Co-authored-by: Victor Lomuller <victor@codeplay.com>

Naghasan

LGTM

elizabethandrews

LGTM

bader · 2021-08-19T06:59:13Z

@AGindinson, @hchilama, @mdtoguchi, ping.

AaronBallman

LGTM

hchilama

LGTM

…4463) #4175 introduced automatic addition of the generic spir64 device target when any section of the input objects had this triple assigned to it. As a result, the actual list of toolchains started exceeding the user-provided one by 1 item. After #4239, the above became a problem. The dispatch of -Xsycl-target-* arguments started happening earlier in theflow, which broke the following use-case: ``` clang++ -fsycl -fsycl-targets=spir64_gen gen-obj.o gen-and-spir64-obj.o -Xsycl-target-backend "-device *" ``` A fix for now is to ignore the autodetected spir64 target when propagating the -Xsycl-target-backend arguments. A permanent solution would involve a re-design of -Xsycl-target-backend handling so that it took place only once in the flow, or belating the addition of the autodetected generic triple into the list of device targets. Signed-off-by: Artem Gindinson <artem.gindinson@intel.com>

npmiller requested review from AGindinson, AaronBallman, bader, elizabethandrews, mdtoguchi, premanandrao and pvchupin as code owners August 3, 2021 14:54

AGindinson reviewed Aug 3, 2021

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

npmiller mentioned this pull request Aug 3, 2021

[SYCL][ROCm] Setup lit tests for ROCm plugin #4163

Merged

bader added the hip Issues related to execution on HIP backend. label Aug 4, 2021

elizabethandrews reviewed Aug 4, 2021

View reviewed changes

clang/include/clang/Basic/DiagnosticDriverKinds.td Outdated Show resolved Hide resolved

npmiller force-pushed the rocm-fix-offload-arch branch 2 times, most recently from 5a790a2 to a634bd9 Compare August 4, 2021 14:09

npmiller and others added 7 commits August 4, 2021 15:13

Update clang/lib/Driver/Driver.cpp

669c331

Co-authored-by: Artem Gindinson <artem.gindinson@intel.com>

[SYCL][ROCm] Fix formatting

1edd3a5

[SYCL][ROCm] Update AMD SYCL offloading test

40bd9df

[SYCL][ROCm] Add test for missing arch diagnostic

ed8b330

[SYCL][ROCm] Add AMD architectures to clang tests

0a01af1

[SYCL][ROCm] Fix missing arch diagnostic formatting

7331029

npmiller force-pushed the rocm-fix-offload-arch branch from a634bd9 to 7331029 Compare August 4, 2021 14:16

bader previously approved these changes Aug 4, 2021

View reviewed changes

malixian reviewed Aug 6, 2021

View reviewed changes

bader requested review from AGindinson and elizabethandrews August 6, 2021 07:11

AGindinson reviewed Aug 6, 2021

View reviewed changes

clang/lib/Driver/Driver.cpp Outdated Show resolved Hide resolved

clang/lib/Driver/Driver.cpp Show resolved Hide resolved

npmiller mentioned this pull request Aug 6, 2021

Update CudaArch structs and functions in clang to something more generic #4279

Closed

elizabethandrews previously approved these changes Aug 6, 2021

View reviewed changes

Update clang/lib/Driver/Driver.cpp

fad9c90

Co-authored-by: Victor Lomuller <victor@codeplay.com>

npmiller dismissed stale reviews from elizabethandrews and bader via fad9c90 August 6, 2021 16:55

bader requested review from AGindinson, Naghasan, bader and elizabethandrews August 11, 2021 09:49

bader approved these changes Aug 11, 2021

View reviewed changes

Naghasan approved these changes Aug 11, 2021

View reviewed changes

elizabethandrews approved these changes Aug 11, 2021

View reviewed changes

AaronBallman approved these changes Aug 19, 2021

View reviewed changes

hchilama approved these changes Aug 19, 2021

View reviewed changes

bader merged commit 2e08d0e into intel:sycl Aug 19, 2021

npmiller mentioned this pull request Aug 30, 2021

[SYCL] Fix AMD architecture flag intel/llvm-test-suite#425

Merged

AGindinson mentioned this pull request Sep 2, 2021

[SYCL][Driver] Fix autodetection of the -Xsycl-target-backend triple #4463

Merged

[SYCL][ROCm] Use offload-arch instead of mcpu for AMD arch #4239

[SYCL][ROCm] Use offload-arch instead of mcpu for AMD arch #4239

Uh oh!

Conversation

npmiller commented Aug 3, 2021

Uh oh!

npmiller commented Aug 3, 2021

Uh oh!

AGindinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

npmiller commented Aug 4, 2021

Uh oh!

malixian Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

npmiller Aug 6, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Naghasan left a comment

Choose a reason for hiding this comment

Uh oh!

elizabethandrews left a comment

Choose a reason for hiding this comment

Uh oh!

bader commented Aug 19, 2021

Uh oh!

AaronBallman left a comment

Choose a reason for hiding this comment

Uh oh!

hchilama left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants