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

npmiller · 2021-07-22T14:56:59Z

This patch wires up the lit tests for the ROCm plugin adding check-sycl-rocm.

check-sycl-rocm includes check-sycl-rocm-on-device and check-sycl-rocm-ptx to run the compiler tests with the NVIDIA triple if using the NVIDIA platform for ROCm or check-sycl-rocm-gcn if using the AMD platform.

This PR is marked as draft is it requires a bit more work to get check-sycl-rocm to work properly.

On AMD it should pass after these two patches are merged/fixed:

On NVIDIA it requires a bit more test filtering and/or debugging because of issues in the HIP APIs, #4123 is one of them but there is a few other bugs that need to be addressed or filtered before this PR can be merged.

bader

LGTM. Thanks!

bader · 2021-07-22T15:28:58Z

sycl/test/CMakeLists.txt

+
+if(SYCL_BUILD_PI_ROCM)
+  add_custom_target(check-sycl-rocm)
+  if("${SYCL_BUILD_PI_ROCM_PLATFORM}" STREQUAL "NVIDIA")


Very minor comment.

See the discussion in #4069. In case we need to support other spellings for SYCL_BUILD_PI_ROCM_PLATFORM, we might need to make sure it's all capital.

Yeah that's probably a good idea, however there's already a bunch of other places that do this comparison. So I'd suggest to keep it like that in this patch and then once it's merged I can put up a new PR updating all of the uses of this to be case-insensitive.

npmiller · 2021-07-23T15:11:24Z

I've marked the remaining two failing tests with NVIDIA as XFAIL, so check-sycl-rocm should now pass on NVIDIA platform with this branch.

bader · 2021-08-03T15:48:48Z

@npmiller, could you resolve merge conflicts, please? It looks like a number of tests were just removed - 7735139.

npmiller · 2021-08-03T16:12:54Z

@npmiller, could you resolve merge conflicts, please? It looks like a number of tests were just removed - 7735139.

Rebased and all conflicts resolved

bader

LGTM, although there is some contradiction between this and another your patch - #4239. I think they suggest using different options for setting target architecture.

npmiller · 2021-08-03T17:14:11Z

LGTM, although there is some contradiction between this and another your patch - #4239. I think they suggest using different options for setting target architecture.

Yes there is, because #4239 changes the option that should be used. The PRs are independent at the moment so either one can go in first but I'll have to update the other to resolve conflict and/or update the new changes.

bader · 2021-08-19T07:00:03Z

@DenisBakhvalov, @kbobrovs, @kychendev, ping.

DenisBakhvalov

Changes in sycl/test/esimd/odr.cpp look good.

kychendev · 2021-08-19T19:23:50Z

Changes in sycl/test/esimd/odr.cpp look good.

LGTM

bader · 2021-08-19T19:41:26Z

On NVIDIA it requires a bit more test filtering and/or debugging because of issues in the HIP APIs, #4123 is one of them but there is a few other bugs that need to be addressed or filtered before this PR can be merged.

We are not doing ROCm plug-in validation on NVIDIA cards, so I'm not sure if it make sense to block the merge of this pull request by #4123.
@npmiller, are you okay to merge this PR before #4123?

npmiller · 2021-08-19T20:40:26Z

On NVIDIA it requires a bit more test filtering and/or debugging because of issues in the HIP APIs, #4123 is one of them but there is a few other bugs that need to be addressed or filtered before this PR can be merged.

We are not doing ROCm plug-in validation on NVIDIA cards, so I'm not sure if it make sense to block the merge of this pull request by #4123.
@npmiller, are you okay to merge this PR before #4123?

Yes, the tests addressed in #4123 are marked as XFAIL here, so this MR should pass check-sycl-rocm on NVidia too at the moment.

I do need to change the -mcpu to --offload-arch now that #4239 is merged.
I'll have a patch to do that tomorrow. If that's easier to review we could merge this and have that patch in a follow up PR, this will mostly work without it, lit will just be a bit annoying as it looks for -mcpu but specifying both flags should work.

bader · 2021-08-20T07:28:27Z

I do need to change the -mcpu to --offload-arch now that #4239 is merged.
I'll have a patch to do that tomorrow. If that's easier to review we could merge this and have that patch in a follow up PR, this will mostly work without it, lit will just be a bit annoying as it looks for -mcpu but specifying both flags should work.

If merging this patch is not urgent, let's make this change here. There should be no difference in where such change is reviewed - within this PR or separately.

npmiller · 2021-08-20T12:41:10Z

Rebased and updated the uses of -mcpu to use the new --offload-arch

bader · 2021-08-20T16:50:48Z

@v-klochkov, it looks like failure in reduction test on Windows is not related to the patch. Please, take a look.
@pvchupin, FYI.

v-klochkov · 2021-08-20T17:53:48Z

@v-klochkov, it looks like failure in reduction test on Windows is not related to the patch. Please, take a look.

Fix for timeout on ACC:
intel/llvm-test-suite#414

…ut (#5032) A follow up to #4163 (comment) `buildbot` script only allows `NVIDIA` capitalise the variable in case user spelled it differently.

bader reviewed Jul 22, 2021

View reviewed changes

npmiller force-pushed the rocm-lit branch from 1e6f314 to d62fdcb Compare July 23, 2021 15:35

npmiller force-pushed the rocm-lit branch from fd137f2 to 95f84ac Compare August 3, 2021 10:55

npmiller marked this pull request as ready for review August 3, 2021 10:59

npmiller requested review from DenisBakhvalov, kbobrovs, pvchupin and a team as code owners August 3, 2021 10:59

npmiller requested a review from sergey-semenov August 3, 2021 10:59

npmiller force-pushed the rocm-lit branch from 95f84ac to ae6b886 Compare August 3, 2021 16:12

bader previously approved these changes Aug 3, 2021

View reviewed changes

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

vladimirlaz previously approved these changes Aug 10, 2021

View reviewed changes

npmiller added 2 commits August 19, 2021 18:02

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

bfad9ae

[SYCL][ROCm] Mark XFAIL two tests failing on ROCm NVIDIA

811395e

DenisBakhvalov previously approved these changes Aug 19, 2021

View reviewed changes

[SYCL][ROCm] Fixup architecture flag in lit tests

4fcb1e7

npmiller dismissed stale reviews from DenisBakhvalov, vladimirlaz, and bader via 4fcb1e7 August 20, 2021 12:40

npmiller force-pushed the rocm-lit branch from ae6b886 to 4fcb1e7 Compare August 20, 2021 12:40

npmiller requested a review from kychendev as a code owner August 20, 2021 12:40

bader approved these changes Aug 20, 2021

View reviewed changes

bader requested a review from vladimirlaz August 20, 2021 16:52

vladimirlaz approved these changes Aug 20, 2021

View reviewed changes

bader merged commit a04da75 into intel:sycl Aug 20, 2021

jchlanda mentioned this pull request Nov 25, 2021

[SYCL] Make sure that SYCL_BUILD_PI_HIP_PLATFORM matches buildbot's case #5032

Merged

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

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

Uh oh!

Conversation

npmiller commented Jul 22, 2021

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

bader Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

npmiller Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

npmiller commented Jul 23, 2021

Uh oh!

bader commented Aug 3, 2021

Uh oh!

npmiller commented Aug 3, 2021

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

npmiller commented Aug 3, 2021

Uh oh!

bader commented Aug 19, 2021

Uh oh!

DenisBakhvalov left a comment

Choose a reason for hiding this comment

Uh oh!

kychendev commented Aug 19, 2021

Uh oh!

bader commented Aug 19, 2021

Uh oh!

npmiller commented Aug 19, 2021

Uh oh!

bader commented Aug 20, 2021

Uh oh!

npmiller commented Aug 20, 2021

Uh oh!

bader commented Aug 20, 2021

Uh oh!

v-klochkov commented Aug 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

v-klochkov commented Aug 20, 2021 •

edited

Loading