[SYCL] Generalize GlobalOffset and enable it for AMDGPU #5855

jchlanda · 2022-03-22T11:45:25Z

The purpose of this patch is to generalize SYCL global offset pass and enable it for AMDGPU.

In order of the commits:

enable global offset in AMD's HIP: 288b48c
decorate SYCL kernel with dedicated MDNode: 0abb088 This removes the need for command line options added by the SYCL driver, discussed here: [SYCL] Generalize local accessor to shared mem pass #5149 (comment)
extract common helpers for local accessor and global offset passes: 6571cdc
generalize the pass: 3e494b5
introduce builtin_amdgcn_implicit_offset and enable the pass for ADMGPU: 5a751a8
implement spirv_GlobalOffset_[x,y,z]: 53f950d
update the docs: 629c977

The main deviation from the NVPTX is the need for supporting address spaces. For AMD kernel arguments reside in constant address space, which for the case with offset forces a copy to private AS, in order to keep the call-graph interface coherent (we can't allocate const address space for the case without offset).

Corresponding test-suit PR: intel/llvm-test-suite#941

bader

libclc changes look okay to me.
It would be interesting to get the feedback from AMDGPU target maintainers on LLVM changes.
@jchlanda, are you planning to upstream new intrinsic and passes?

jchlanda · 2022-03-22T12:51:25Z

libclc changes look okay to me. It would be interesting to get the feedback from AMDGPU target maintainers on LLVM changes. @jchlanda, are you planning to upstream new intrinsic and passes?

I'm not sure if it would ever make sense outside of SYCL. And it relies on the metadata generated through SYCL driver and SYCLLowerIR.

sycl/doc/design/CompilerAndRuntimeDesign.md

vmaksimo · 2022-03-23T10:03:07Z

Hi @jchlanda and reviewers!
When is it planned to merge the change? The related test-suite PR has been already merged, and it is causing the test failure in CI runs in SYCL :: Basic/parallel_for_indexers.cpp
UPD: the merged change was reverted. Please merge the test-suite change (intel/llvm-test-suite#941) simultaneously with this PR.

jchlanda · 2022-03-23T10:12:33Z

Hi @jchlanda and reviewers! When is it planned to merge the change? The related test-suite PR has been already merged, and it is causing the test failure in CI runs in SYCL :: Basic/parallel_for_indexers.cpp UPD: the merged change was reverted. Please merge the test-suite change (intel/llvm-test-suite#941) simultaneously with this PR.

Yeap, will do that, thanks.

clang/lib/CodeGen/TargetInfo.cpp

clang/include/clang/Basic/BuiltinsAMDGPU.def

clang/lib/CodeGen/TargetInfo.cpp

elizabethandrews

Sorry I didn't notice there were no FE tests earlier. Can you add a FE test since you're changing CodeGen?

jchlanda · 2022-03-29T08:34:10Z

Sorry I didn't notice there were no FE tests earlier. Can you add a FE test since you're changing CodeGen?

@elizabethandrews
added a test for the annotations here: c450a3b87f819ef90da3c492ede36fe76bba6889

elizabethandrews

Thanks! FE changes LGTM

smanna12

FE changes Ok.

jchlanda · 2022-04-01T10:09:20Z

@bader would you like me to do anything else for this PR?

SYCL :: api/simd_any_all.cpp
SYCL :: regression/big_const_initializer.cpp

look unrelated, KernelAndProgram/undefined-symbol.cpp is // XFAIL: cuda || hip || (opencl && gpu) on the top of intelbranch and Basic/parallel_for_indexers.cpp will be supported with intel/llvm-test-suite#941 going in.

bader · 2022-04-01T10:16:27Z

@jchlanda, two things: please, resolve merge conflicts and ping reviewers. I see approvals only from front-end team and Greg, so merge tip of the sycl branch and re-request the review.

v-klochkov · 2022-05-04T04:33:26Z

sycl/doc/design/CompilerAndRuntimeDesign.md

+In order to emulate this and make generated kernel compliant, an intrinsic
+`llvm.nvvm.implicit.offset` (clang builtin `__builtin_ptx_implicit_offset`) was
+introduced materializing the use of this implicit parameter for the NVPTX
+backend. AMDGCN uses the same approach with `llvm.andgpu.implicit.offset` and


non-blocking comment (may be fixed in separate PR. It does not deserve a fix that would dismiss existing approvals):
Is it a misprint here? "llvm.andgpu." -> "llvm.amdgpu."

Yeap, that's a typo, will fix now.

v-klochkov · 2022-05-04T04:44:26Z

@asudarsa would you please review the changes in files owned by @intel/dpcpp-tools-reviewers . I see that automatically @psamolysov-intel assigned is out-of-office for unknown time.

v-klochkov · 2022-05-04T04:51:20Z

@jchlanda - if you decide to update the patch again, then please use "git merge ...; git push ..." instead of "git rebase ...; git push -f ...".
(rebase + force-push) clears all reviewers approvals, while 'merge + push' updates the patch to head and preserves the approvals.

pvchupin · 2022-05-04T20:08:05Z

@gmlueck, can you review please?

clang/lib/CodeGen/TargetInfo.cpp

llvm/lib/SYCLLowerIR/GlobalOffset.cpp

asudarsa

Few nitpicks. But looks good overall. Thanks

jchlanda · 2022-05-05T10:36:30Z

@jchlanda - if you decide to update the patch again, then please use "git merge ...; git push ..." instead of "git rebase ...; git push -f ...". (rebase + force-push) clears all reviewers approvals, while 'merge + push' updates the patch to head and preserves the approvals.

My bad, sorry.

jchlanda · 2022-05-05T13:35:36Z

https://github.com/intel/llvm/runs/6305612506?check_suite_focus=true#step:8:1928 will be resolved with intel/llvm-test-suite#941

asudarsa

LGTM. Thanks

* Fix links to configure.py and compile.py * Linking to the file in tree caused the link in the docs to just download the python script. It makes more sense to link to the github web UI for these as they should be used in a checkout anyway. * Remove CUDA requiring device selector * Since intel#6203 the device selector should handle this case well. * Update HIP backend limitations * HIP is no longer in beta * Windows isn't supported but intel#17702 made the build work with it so it might work for some users. * Global offset has been supported since intel#5855 * Add common limitations * Update HIP for Nvidia section * This might work but is not supported * Update HIP section * Recommended HIP version + testing platforms * HIP is no longer in beta * Add note on target aliases for CUDA and HIP

* Fix links to configure.py and compile.py * Linking to the file in tree caused the link in the docs to just download the python script. It makes more sense to link to the github web UI for these as they should be used in a checkout anyway. * Remove CUDA requiring device selector * Since #6203 the device selector should handle this case well. * Update HIP backend limitations * HIP is no longer in beta * Windows isn't supported but #17702 made the build work with it so it might work for some users. * Global offset has been supported since #5855 * Add common limitations * Update HIP for Nvidia section * This might work but is not supported * Update HIP section * Recommended HIP version + testing platforms * HIP is no longer in beta * Add note on target aliases for CUDA and HIP

jchlanda requested review from a team and bader as code owners March 22, 2022 11:45

jchlanda requested a review from v-klochkov March 22, 2022 11:45

bader previously approved these changes Mar 22, 2022

View reviewed changes

jchlanda dismissed bader’s stale review via 593e51f March 22, 2022 12:15

jchlanda force-pushed the jakub/global_offset_sycl branch from 629c977 to 593e51f Compare March 22, 2022 12:15

gmlueck reviewed Mar 22, 2022

View reviewed changes

sycl/doc/design/CompilerAndRuntimeDesign.md Show resolved Hide resolved

vladimirlaz mentioned this pull request Mar 23, 2022

[SYCL] Enable parallel indexers for AMD, after adding global offset intel/llvm-test-suite#941

Merged

elizabethandrews reviewed Mar 24, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Outdated Show resolved Hide resolved

jchlanda force-pushed the jakub/global_offset_sycl branch 2 times, most recently from 33031fe to f4c4abb Compare March 25, 2022 08:10

smanna12 reviewed Mar 25, 2022

View reviewed changes

clang/include/clang/Basic/BuiltinsAMDGPU.def Outdated Show resolved Hide resolved

smanna12 reviewed Mar 25, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Outdated Show resolved Hide resolved

elizabethandrews reviewed Mar 25, 2022

View reviewed changes

jchlanda force-pushed the jakub/global_offset_sycl branch 2 times, most recently from d0c99fe to c450a3b Compare March 29, 2022 09:29

elizabethandrews previously approved these changes Mar 29, 2022

View reviewed changes

smanna12 previously approved these changes Mar 29, 2022

View reviewed changes

jchlanda dismissed smanna12’s stale review via 41f2152 April 1, 2022 10:35

v-klochkov reviewed May 4, 2022

View reviewed changes

v-klochkov previously approved these changes May 4, 2022

View reviewed changes

v-klochkov requested a review from asudarsa May 4, 2022 04:44

mdtoguchi previously approved these changes May 4, 2022

View reviewed changes

pvchupin previously approved these changes May 4, 2022

View reviewed changes

asudarsa reviewed May 5, 2022

View reviewed changes

clang/lib/CodeGen/TargetInfo.cpp Show resolved Hide resolved

asudarsa reviewed May 5, 2022

View reviewed changes

llvm/lib/SYCLLowerIR/GlobalOffset.cpp Outdated Show resolved Hide resolved

asudarsa reviewed May 5, 2022

View reviewed changes

llvm/lib/SYCLLowerIR/GlobalOffset.cpp Show resolved Hide resolved

asudarsa previously approved these changes May 5, 2022

View reviewed changes

PR comments addressed.

2c433e9

jchlanda dismissed stale reviews from asudarsa, pvchupin, mdtoguchi, v-klochkov, smanna12, and elizabethandrews via 2c433e9 May 5, 2022 11:22

jchlanda requested review from v-klochkov and asudarsa May 5, 2022 13:33

v-klochkov approved these changes May 5, 2022

View reviewed changes

asudarsa approved these changes May 5, 2022

View reviewed changes

pvchupin merged commit 2dc3c06 into intel:sycl May 5, 2022

npmiller mentioned this pull request Nov 27, 2023

[SYCL][HIP] Implement proper support for gloal offsets #4906

Closed

npmiller mentioned this pull request Apr 9, 2025

[SYCL][DOC] CUDA and HIP GetStartedGuide updates #17928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL] Generalize GlobalOffset and enable it for AMDGPU #5855

[SYCL] Generalize GlobalOffset and enable it for AMDGPU #5855

jchlanda commented Mar 22, 2022 •

edited by bader

Loading

bader left a comment

jchlanda commented Mar 22, 2022

vmaksimo commented Mar 23, 2022 •

edited

Loading

jchlanda commented Mar 23, 2022

elizabethandrews left a comment

jchlanda commented Mar 29, 2022 •

edited

Loading

elizabethandrews left a comment

smanna12 left a comment

jchlanda commented Apr 1, 2022

bader commented Apr 1, 2022

v-klochkov May 4, 2022 •

edited

Loading

jchlanda May 5, 2022

v-klochkov commented May 4, 2022 •

edited

Loading

v-klochkov commented May 4, 2022

pvchupin commented May 4, 2022

asudarsa left a comment

jchlanda commented May 5, 2022

jchlanda commented May 5, 2022

asudarsa left a comment

[SYCL] Generalize GlobalOffset and enable it for AMDGPU #5855

[SYCL] Generalize GlobalOffset and enable it for AMDGPU #5855

Conversation

jchlanda commented Mar 22, 2022 • edited by bader Loading

bader left a comment

Choose a reason for hiding this comment

jchlanda commented Mar 22, 2022

vmaksimo commented Mar 23, 2022 • edited Loading

jchlanda commented Mar 23, 2022

elizabethandrews left a comment

Choose a reason for hiding this comment

jchlanda commented Mar 29, 2022 • edited Loading

elizabethandrews left a comment

Choose a reason for hiding this comment

smanna12 left a comment

Choose a reason for hiding this comment

jchlanda commented Apr 1, 2022

bader commented Apr 1, 2022

v-klochkov May 4, 2022 • edited Loading

Choose a reason for hiding this comment

jchlanda May 5, 2022

Choose a reason for hiding this comment

v-klochkov commented May 4, 2022 • edited Loading

v-klochkov commented May 4, 2022

pvchupin commented May 4, 2022

asudarsa left a comment

Choose a reason for hiding this comment

jchlanda commented May 5, 2022

jchlanda commented May 5, 2022

asudarsa left a comment

Choose a reason for hiding this comment

jchlanda commented Mar 22, 2022 •

edited by bader

Loading

vmaksimo commented Mar 23, 2022 •

edited

Loading

jchlanda commented Mar 29, 2022 •

edited

Loading

v-klochkov May 4, 2022 •

edited

Loading

v-klochkov commented May 4, 2022 •

edited

Loading