[SYCL][CUDA] Adds PI CUDA support for reqd_work_group_size attribute #3735

steffenlarsen · 2021-05-12T13:20:14Z

This commit adds support for reqd_work_group_size in the PI CUDA backend
by extracting the attribute as program metadata. The program metadata
accompanies the binary when passed to the backend and it is up to the
backend if they extract any useful metadata. This adds two additional
parameters to piProgramCreateWithBinary for passing the program
metadata.

Program metadata is transported as a properties created by
sycl-post-link, so this commit also changes the behaviour of the NVPTX
path for linkage actions leading to the offload wrapper. These changes
uses file tables for the NVPTX path as well to allow generation and
preservation of properties. This assumes that the file table only ever
contains a single row if taking the NVPTX path and will fail otherwise.

steffenlarsen · 2021-05-12T13:42:29Z

Fixes test SYCL/Basic/reqd_work_group_size.cpp in test suite for CUDA. (PR: intel/llvm-test-suite#278)

sycl/unittests/pi/cuda/test_kernels.cpp

sycl/plugins/cuda/pi_cuda.cpp

steffenlarsen · 2021-05-13T14:16:01Z

Looks like I missed a clang driver test. Should be fixed now.

AlexeySachkov

Code review for PropertySetIO, file-table-tform and sycl-post-link: overall changes look good to me, just a few (mostly minor) comments

llvm/tools/sycl-post-link/sycl-post-link.cpp

llvm/tools/file-table-tform/file-table-tform.cpp

llvm/lib/Support/SimpleTable.cpp

llvm/tools/file-table-tform/file-table-tform.cpp

llvm/tools/sycl-post-link/sycl-post-link.cpp

mdtoguchi

Documentation (device link and wrap image) should be updated as well. https://github.com/intel/llvm/blob/sycl/sycl/doc/images/DeviceLinkAndWrap.svg

steffenlarsen · 2021-05-18T14:43:45Z

Documentation (device link and wrap image) should be updated as well. https://github.com/intel/llvm/blob/sycl/sycl/doc/images/DeviceLinkAndWrap.svg

Definitely! I have updated the documentation. Hopefully I found all the relevant sections.

pvchupin

LGTM. @kbobrovs please take a look as well.

smaslov-intel · 2021-05-18T22:30:18Z

This commit adds support for reqd_work_group_size in the PI CUDA backend
by extracting the attribute as program metadata. The program metadata
accompanies the binary when passed to the backend and it is up to the
backend if they extract any useful metadata. This adds two additional
parameters to piProgramCreateWithBinary for passing the program
metadata.

How is this attribute supported when program is created with SPIR-V?

steffenlarsen · 2021-05-19T09:18:58Z

How is this attribute supported when program is created with SPIR-V?

I have not checked with L0, but OpenCL can both query information about reqd_work_group_size of a kernel using the PI_KERNEL_GROUP_INFO_COMPILE_WORK_GROUP_SIZE descriptor and will explicitly fail with CL_INVALID_WORK_GROUP_SIZE if the attribute restrictions are not met during launch.

PTX has reqntid (which reqd_work_group_size is mapped to with #3755), but the documentation is vague about the specific error behavior when it is not met. Likewise, the CUDA driver API does not expose a way of querying this information. This is the motivation for carrying metadata to the runtime, more or less.

In case either OpenCL or L0 may be in need of any similar program metadata, it should be as simple as just making sycl-post-link generate the information, append it to PropSet[llvm::util::PropertySetRegistry::SYCL_PROGRAM_METADATA] and flip the switch so that it is generated for their path. Then it should reach the backend through piProgramCreateWithBinary.

pvchupin · 2021-07-16T05:56:19Z

@steffenlarsen, could you please resolve conflicts. I think everybody required reviewed and approved at least once.
@mdtoguchi, @AlexeySachkov, @kbobrovs, please comment if anything left.

steffenlarsen · 2021-07-16T10:34:23Z

Thanks @pvchupin . Sorry for the double rebase. New conflicts snuck in while I was doing the first rebase. It should be ready now.

bader · 2021-07-16T10:56:56Z

Ouch... @steffenlarsen, sorry, I seem to introduce another merge conflict. Could you take a look, please?

This commit adds support for reqd_work_group_size in the PI CUDA backend by extracting the attribute as program metadata. The program metadata accompanies the binary when passed to the backend and it is up to the backend if they extract any useful metadata. This adds two additional parameters to piProgramCreateWithBinary for passing the program metadata. Program metadata is transported as a properties created by sycl-post-link, so this commit also changes the behaviour of the NVPTX path for linkage actions leading to the offload wrapper. These changes uses file tables for the NVPTX path as well to allow generation and preservation of properties. This assumes that the file table only ever contains a single row if taking the NVPTX path and will fail otherwise. Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

…consistent Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

steffenlarsen · 2021-07-16T11:40:30Z

Ouch... @steffenlarsen, sorry, I seem to introduce another merge conflict. Could you take a look, please?

Haha, no worries! It has been taken care of. Thanks for letting me know.

kbobrovs

driver, doc, file-table-tform LGTM

steffenlarsen requested review from AGindinson, AlexeySachkov, bader, kbobrovs, mdtoguchi, mlychkov, smaslov-intel and a team as code owners May 12, 2021 13:20

bader added the cuda CUDA back-end label May 12, 2021

steffenlarsen mentioned this pull request May 12, 2021

[SYCL][CUDA] Enables reqd_work_group_test for CUDA intel/llvm-test-suite#278

Merged

bader previously approved these changes May 13, 2021

View reviewed changes

sycl/unittests/pi/cuda/test_kernels.cpp Show resolved Hide resolved

sycl/plugins/cuda/pi_cuda.cpp Outdated Show resolved Hide resolved

steffenlarsen dismissed bader’s stale review via 37c1652 May 13, 2021 14:14

bader previously approved these changes May 13, 2021

View reviewed changes

steffenlarsen mentioned this pull request May 13, 2021

[WIP][SYCL-PTX] Generate reqntid PTX directive from reqd_work_group_size #3755

Closed

AlexeySachkov reviewed May 14, 2021

View reviewed changes

steffenlarsen dismissed bader’s stale review via 531627d May 17, 2021 09:22

bader previously approved these changes May 17, 2021

View reviewed changes

mlychkov reviewed May 17, 2021

View reviewed changes

llvm/tools/file-table-tform/file-table-tform.cpp Outdated Show resolved Hide resolved

llvm/tools/sycl-post-link/sycl-post-link.cpp Show resolved Hide resolved

steffenlarsen dismissed bader’s stale review via a22991d May 17, 2021 11:32

mdtoguchi reviewed May 17, 2021

View reviewed changes

steffenlarsen requested a review from pvchupin as a code owner May 18, 2021 14:41

pvchupin reviewed May 18, 2021

View reviewed changes

pvchupin previously approved these changes May 18, 2021

View reviewed changes

pvchupin previously approved these changes Jul 16, 2021

View reviewed changes

steffenlarsen dismissed stale reviews from pvchupin, smaslov-intel, and vladimirlaz via 79632f4 July 16, 2021 10:02

steffenlarsen force-pushed the steffen/cuda_reqd_wg_size branch 2 times, most recently from 79632f4 to 93a35ad Compare July 16, 2021 10:31

Steffen Larsen added 12 commits July 16, 2021 12:26

Fix driver offload test and minor changes

de11767

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Adjusting for feedback and more testing

a6ccafc

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Remove redundant check

4b04b37

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Documentation changed to reflect post-link step changes

5e829f1

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Make copy_single_file a transformation

40dd00c

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Adjusts updateCellValue parameters, comments, and errors

25eef45

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Change code-splitting TODO

c952da0

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Added assertion for 3 reqd_work_group_size metadata operands

177c14d

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Adds piProgramCreateWithBinary comment and makes new parameters more …

c817325

…consistent Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Fix formatting

76ad746

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

Fix sycl-offload-amdgcn test

5fda49a

Signed-off-by: Steffen Larsen <steffen.larsen@codeplay.com>

steffenlarsen force-pushed the steffen/cuda_reqd_wg_size branch from 93a35ad to 5fda49a Compare July 16, 2021 11:37

bader approved these changes Jul 16, 2021

View reviewed changes

kbobrovs approved these changes Jul 16, 2021

View reviewed changes

pvchupin approved these changes Jul 16, 2021

View reviewed changes

bader merged commit a8fe4a5 into intel:sycl Jul 16, 2021

sergey-semenov mentioned this pull request Jul 21, 2021

Program with device code in multiple translation units fails on CUDA #4156

Closed

AidanBeltonS mentioned this pull request Aug 26, 2021

Clang crashes since last pulldowns when building shared objects #4294

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][CUDA] Adds PI CUDA support for reqd_work_group_size attribute #3735

[SYCL][CUDA] Adds PI CUDA support for reqd_work_group_size attribute #3735

steffenlarsen commented May 12, 2021

steffenlarsen commented May 12, 2021

steffenlarsen commented May 13, 2021

AlexeySachkov left a comment

mdtoguchi left a comment

steffenlarsen commented May 18, 2021

pvchupin left a comment

smaslov-intel commented May 18, 2021

steffenlarsen commented May 19, 2021

pvchupin commented Jul 16, 2021

steffenlarsen commented Jul 16, 2021

bader commented Jul 16, 2021

steffenlarsen commented Jul 16, 2021

kbobrovs left a comment

[SYCL][CUDA] Adds PI CUDA support for reqd_work_group_size attribute #3735

[SYCL][CUDA] Adds PI CUDA support for reqd_work_group_size attribute #3735

Conversation

steffenlarsen commented May 12, 2021

steffenlarsen commented May 12, 2021

steffenlarsen commented May 13, 2021

AlexeySachkov left a comment

Choose a reason for hiding this comment

mdtoguchi left a comment

Choose a reason for hiding this comment

steffenlarsen commented May 18, 2021

pvchupin left a comment

Choose a reason for hiding this comment

smaslov-intel commented May 18, 2021

steffenlarsen commented May 19, 2021

pvchupin commented Jul 16, 2021

steffenlarsen commented Jul 16, 2021

bader commented Jul 16, 2021

steffenlarsen commented Jul 16, 2021

kbobrovs left a comment

Choose a reason for hiding this comment