[SYCL][HIP] Rename the ROCm plugin to HIP #4418

npmiller · 2021-08-27T14:51:45Z

HIP is a better name in this case as it is the API the plugin is
actually built on rather than ROCm which is more of a framework.

As discussed this patch renames the ROCm plugin to HIP, it is thus very large but doesn't have any functional changes.

I tested it on both AMD and NVIDIA with check-sycl-hip and check-sycl-hip-on-device, and the results were the same than before this patch with check-sycl-rocm and check-sycl-rocm-on-device.

steffenlarsen

Good stuff! Better get the renaming done sooner rather than later. Very minor comment that has little to do with these changes.

I am a bit on the fence about calling it "HIP AMD" and "HIP NVIDIA". Would "HIP ROCm" and "HIP CUDA" be more fitting?
I assume a similar patch is coming for https://github.com/intel/llvm-test-suite?

steffenlarsen · 2021-08-27T16:34:32Z

Ping @malixian

npmiller · 2021-08-27T17:26:43Z

Good stuff! Better get the renaming done sooner rather than later. Very minor comment that has little to do with these changes.

I am a bit on the fence about calling it "HIP AMD" and "HIP NVIDIA". Would "HIP ROCm" and "HIP CUDA" be more fitting?
I assume a similar patch is coming for https://github.com/intel/llvm-test-suite?

I went for that because the HIP defines are __HIP_PLATFORM_AMD__, and __HIP_PLATFORM_NVIDIA__ (they also have __HIP_PLATFORM_HCC__ and __HIP_PLATFORM_NVCC__ but I believe these might be deprecated). But I don't have strong opinions on this.

And yeas I'll have a PR for llvm-test-suite soon although it should still work fine with this patch.

bader

LGTM. Just a couple of minor suggestions.

I also created a merge conflict by merging #4393, but it should be easy to resolve.

buildbot/configure.py

sycl/doc/GetStartedGuide.md

sycl/test/on-device/group_algorithms_sycl2020/permute_select.cpp

npmiller · 2021-08-30T10:48:18Z

Matching PR in llvm-test-suite:

[SYCL] Rename ROCm plugin to HIP llvm-test-suite#426

steffenlarsen

LGTM. Thanks for fixing the tests in the test-suite as well.

npmiller · 2021-09-02T11:35:41Z

I just fixed the backend name in the new test that showed up in the CI

kbobrovs · 2021-09-02T14:51:49Z

sycl/test/esimd/odr.cpp

@@ -12,7 +12,7 @@
 // UNSUPPORTED: cuda
 //
 // Linking issues with AMD:
-// XFAIL: rocm_amd
+// XFAIL: hip_amd


This should rather be UNSUPPORTED: cuda,hip_amd, I believe, as esimd is only for Intel GPU.

Oh I see, updated to UNSUPPORTED: cuda || hip

I just realized hip is not a lit feature in this repo, it is in llvm-test-suite though, changed to hip_amd for now, the lit setup for this may require some refactoring in the future, it was a bit thrown off by the on-device tests move.

kbobrovs

sycl/test/esimd/odr.cpp LGTM

npmiller · 2021-09-03T14:53:55Z

Rebased and fixed merge conflicts

HIP is a better name in this case as it is the API the plugin is actually built on rather than ROCm which is more of a framework.

Co-authored-by: Alexey Bader <alexey.bader@intel.com>

hip is not a lit feature in the main repo, hip_amd is

npmiller · 2021-09-13T09:56:46Z

Rebased and conflicts resolved

bader · 2021-09-13T14:29:24Z

FYI: Jenkins/Precommit fails on SYCL :: FilterSelector/select.cpp.

llvm-test-suite/SYCL/FilterSelector/select.cpp:54:36: error: no member named 'rocm' in 'sycl::backend'
} else if (Backend == backend::rocm) {

intel/llvm-test-suite#426 should fix this error.

- Change from intel/llvm#4418

npmiller requested review from bader, DenisBakhvalov, kbobrovs, kychendev, pvchupin, smaslov-intel and a team as code owners August 27, 2021 14:51

steffenlarsen previously approved these changes Aug 27, 2021

View reviewed changes

bader reviewed Aug 30, 2021

View reviewed changes

buildbot/configure.py Outdated Show resolved Hide resolved

sycl/doc/GetStartedGuide.md Outdated Show resolved Hide resolved

sycl/test/on-device/group_algorithms_sycl2020/permute_select.cpp Outdated Show resolved Hide resolved

npmiller dismissed steffenlarsen’s stale review via a01dbea August 30, 2021 09:22

npmiller force-pushed the rocm-hip branch from eb67cd5 to c60e028 Compare August 30, 2021 10:07

bader previously approved these changes Aug 30, 2021

View reviewed changes

npmiller mentioned this pull request Aug 30, 2021

[SYCL] Rename ROCm plugin to HIP intel/llvm-test-suite#426

Merged

bader dismissed their stale review via 07a8194 September 2, 2021 10:28

bader previously approved these changes Sep 2, 2021

View reviewed changes

bader requested a review from steffenlarsen September 2, 2021 10:29

steffenlarsen previously approved these changes Sep 2, 2021

View reviewed changes

npmiller dismissed stale reviews from steffenlarsen and bader via b0246ad September 2, 2021 11:35

steffenlarsen previously approved these changes Sep 2, 2021

View reviewed changes

bader previously approved these changes Sep 2, 2021

View reviewed changes

kbobrovs reviewed Sep 2, 2021

View reviewed changes

npmiller dismissed stale reviews from bader and steffenlarsen via 8640daf September 2, 2021 14:56

bader previously approved these changes Sep 2, 2021

View reviewed changes

kbobrovs previously approved these changes Sep 2, 2021

View reviewed changes

steffenlarsen previously approved these changes Sep 2, 2021

View reviewed changes

npmiller dismissed stale reviews from steffenlarsen, kbobrovs, and bader via 4e3df36 September 2, 2021 17:45

bader previously approved these changes Sep 2, 2021

View reviewed changes

npmiller dismissed bader’s stale review via 50d6b2b September 3, 2021 14:45

npmiller force-pushed the rocm-hip branch from 4e3df36 to 50d6b2b Compare September 3, 2021 14:45

npmiller and others added 6 commits September 13, 2021 10:49

[SYCL][HIP] Rename the ROCm plugin to HIP

de68e7c

HIP is a better name in this case as it is the API the plugin is actually built on rather than ROCm which is more of a framework.

Update buildbot/configure.py

6cae68c

Co-authored-by: Alexey Bader <alexey.bader@intel.com>

Update sycl/doc/GetStartedGuide.md

86cf7fe

Co-authored-by: Alexey Bader <alexey.bader@intel.com>

[SYCL][HIP] Update backend name in new test case

20dabe7

[SYCL][HIP] Mark esimd tests as unsupported for HIP

288a5c9

[SYCL][HIP] Fix hip lit feature

35b827d

hip is not a lit feature in the main repo, hip_amd is

npmiller force-pushed the rocm-hip branch from 50d6b2b to 35b827d Compare September 13, 2021 09:51

bader requested review from steffenlarsen and kbobrovs September 13, 2021 10:22

bader approved these changes Sep 13, 2021

View reviewed changes

steffenlarsen approved these changes Sep 13, 2021

View reviewed changes

smaslov-intel approved these changes Sep 13, 2021

View reviewed changes

bader merged commit facec17 into intel:sycl Sep 13, 2021

dongkyunahn-intel added a commit to dongkyunahn-intel/llvm-test-suite that referenced this pull request Sep 14, 2021

Renaming ROCm with HIP

1a8f6b5

- Change from intel/llvm#4418

[SYCL][HIP] Rename the ROCm plugin to HIP #4418

[SYCL][HIP] Rename the ROCm plugin to HIP #4418

Uh oh!

Conversation

npmiller commented Aug 27, 2021

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

steffenlarsen commented Aug 27, 2021

Uh oh!

npmiller commented Aug 27, 2021

Uh oh!

bader left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

npmiller commented Aug 30, 2021

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

npmiller commented Sep 2, 2021

Uh oh!

kbobrovs Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

npmiller Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

npmiller Sep 2, 2021

Choose a reason for hiding this comment

Uh oh!

kbobrovs left a comment

Choose a reason for hiding this comment

Uh oh!

npmiller commented Sep 3, 2021

Uh oh!

npmiller commented Sep 13, 2021

Uh oh!

bader commented Sep 13, 2021

Uh oh!

Uh oh!