[UR][L0] Add support for passing device list to urProgramBuild/Link/Compile #934

nrspruit · 2023-10-06T23:53:12Z

piProgramBuild receives a list of devices, while urProgramBuild does not. This produces a series of issues when a UR program needs to be created for a specific device.

So define a new API, called urProgramBuildExp to pass this list.

…Link/Compile pre-commit check of oneapi-src/unified-runtime#934 piProgramBuild receives a list of devices, while urProgramBuild does not. This produces a series of issues when a UR program needs to be created for a specific device. So define a new API, called urProgramBuildExp to pass this list. Signed-off-by: Spruit, Neil R <[email protected]>

source/adapters/cuda/program.cpp

source/adapters/hip/program.cpp

jandres742 · 2023-10-10T03:41:34Z

thanks @nrspruit . Changes look good. Please check failures.

jandres742 · 2023-10-10T04:44:32Z

we have now 3 similar PRs for this

#919
#924
#934

which one we will be using?

kbenzie · 2023-10-10T11:50:07Z

which one we will be using?

see #924 (comment)

jchlanda

CUDA/HIP 👍

kbenzie

We'll need to bring in the spec changes for this first, then add these entry points to the loader dispatch tables.

nrspruit · 2023-11-13T19:06:52Z

Hello @kbenzie ,

With #924 (comment) 924 merged, we need to merge this change, then update the PR intel/llvm#11464 with the updated unified runtime version to merge the fix to the latest version.

mmoadeli · 2023-11-13T22:09:20Z

source/adapters/level_zero/program.cpp

  ze_context_handle_t ZeContext = Program->Context->ZeContext;
+  std::ignore = Context;
+  std::ignore = numDevices;


I'd remove Context and numDevices in function definition, i.e. to have argument type but not argument name. If it's not used in the function body.

thanks @mmoadeli . I would prefer to leave them. That way, urProgramBuild and urProgramBuildExp are as compatible as possible, allowing us to see if in the future we can merge the two into a single API, or find other use cases.

For instance, if in urProgramBuildExp numDevices is 0, then the program could be built for all devices in the Context instead, which is kind of falling back again to urProgramBuild.

what do you think?

thanks @mmoadeli . I would prefer to leave them. That way, urProgramBuild and urProgramBuildExp are as compatible as possible, allowing us to see if in the future we can merge the two into a single API, or find other use cases.

For instance, if in urProgramBuildExp numDevices is 0, then the program could be built for all devices in the Context instead, which is kind of falling back again to urProgramBuild.

what do you think?
I did not mean to change the API, only changing it to below in this case, to have the type for consistency and not to have the name to avoid using std::ignore.

UR_APIEXPORT ur_result_t UR_APICALL urProgramBuildExp( ur_context_handle_t, ///< [in] handle of the context instance. ur_program_handle_t Program, ///< [in] Handle of the program to build. uint32_t, ur_device_handle_t *phDevices, const char *Options ///< [in][optional] pointer to build options ///< null-terminated string. )

kbenzie · 2023-11-14T11:11:49Z

Hello @kbenzie ,

With #924 (comment) 924 merged, we need to merge this change, then update the PR intel/llvm#11464 with the updated unified runtime version to merge the fix to the latest version.

The reason this can't be merged yet is because the spec changes to add these entry point don't exist in the adapters branch. We'd like to get those changes into the adapters branch with #1072 but it depends on #1014 and #1016. Both of these are pulling in lots of extra stuff which is prooving to be a real pain to integrate, especially the Windows E2E test failures in intel/llvm#11771 (to pull in the changes in #1014). Any help you can provide getting these to merge faster will mean this can be merged sooner.

kbenzie · 2023-11-15T16:03:38Z

Hello @kbenzie ,
With #924 (comment) 924 merged, we need to merge this change, then update the PR intel/llvm#11464 with the updated unified runtime version to merge the fix to the latest version.

The reason this can't be merged yet is because the spec changes to add these entry point don't exist in the adapters branch. We'd like to get those changes into the adapters branch with #1072 but it depends on #1014 and #1016. Both of these are pulling in lots of extra stuff which is prooving to be a real pain to integrate, especially the Windows E2E test failures in intel/llvm#11771 (to pull in the changes in #1014). Any help you can provide getting these to merge faster will mean this can be merged sooner.

Since we've had trouble with getting the spec changes from main into the adapters branch we are going with plan B which is to cherry-pick the spec changes needed for this PR to the adapters branch in #1083 instead.

kbenzie · 2023-11-16T17:20:56Z

I'm going to pull #1083 into this branch and also add another commit to include these entry points in the UR loader DDI table. Having these in one place will make it easier to pull into intel/llvm once we get a chance to do so (currently there are issues in CI).

FYI this will involve force pushing to your branch.

piProgramBuild receives a list of devices, while urProgramBuild does not. This produces a series of issues when a UR program needs to be created for a specific device. So define a new API, called urProgramBuildExp to pass this list. Authored-by: [email protected]

Expand upon the introduction of `urProgramBuildExp` and include `urProgramCompileExp` and `urProgramLinkExp` which include a device-list in place of a context. These more closely align with the PI/OpenCL analogues but only to introduce device-lists, not all extant arguments from those entry-points. This patch also moves the `urProgramBuildExp` definition into an experimental feature file and introduces a brief document containing motivation.

…ompile piProgramBuild receives a list of devices, while urProgramBuild does not. This produces a series of issues when a UR program needs to be created for a specific device. So define a new API, called urProgramBuildExp to pass this list. Signed-off-by: Spruit, Neil R <[email protected]> Co-authored-by: Jaime Arteaga <[email protected]>

Add the `urProgramBuildExp`, `urProgramCompileExp`, and `urProgramLinkExp` to the loader `ur_program_exp_dditable_t`. Also add `"ur_exp_multi_device_compile"` to the list of extensions supported by the L0 adapter, enables the SYCL RT to query support.

Align the `urProgramLinkExp` spec and implementation argument orders to fix Windows link error.

kbenzie · 2023-11-16T18:29:50Z

So I'm learning that these implementations have different arguments to the entry-points defined in the spec.

jandres742 · 2023-11-16T23:00:26Z

So I'm learning that these implementations have different arguments to the entry-points defined in the spec.

@nrspruit : could you check?

nrspruit · 2023-11-17T01:02:25Z

So I'm learning that these implementations have different arguments to the entry-points defined in the spec.

@nrspruit : could you check?

Apologies, yes the implementation was for the earlier version of the API, I have fixed it now and the build is passing. Please confirm that I did not miss anything.

jandres742 · 2023-11-17T04:41:55Z

include/ur.py

@@ -196,6 +196,9 @@ class ur_function_v(IntEnum):
    ADAPTER_RETAIN = 179                            ## Enumerator for ::urAdapterRetain
    ADAPTER_GET_LAST_ERROR = 180                    ## Enumerator for ::urAdapterGetLastError
    ADAPTER_GET_INFO = 181                          ## Enumerator for ::urAdapterGetInfo
+    PROGRAM_BUILD_EXP = 197                         ## Enumerator for ::urProgramBuildExp


dont we have all the changes in spec already merged in #924? I would expect this PR to only need to modify code in the L0 adapter, as the spec part has been already taken care of. Or is that this is not rebased?

#924 was merged to main not adapters, we created #1083 to cherry-pick the spec changes into adapters since we were having trouble syncing adapters and main. I then made the decision to take the cherry-pick from #1083 into this PR to reduce the number of intel/llvm PR's required to get this feature merged in time for the next release.

source/adapters/level_zero/program.cpp

…nition to match spec Signed-off-by: Spruit, Neil R <[email protected]>

…ompile piProgramBuild receives a list of devices, while urProgramBuild does not. This produces a series of issues when a UR program needs to be created for a specific device. So define a new API, called urProgramBuildExp to pass this list. Requires related patch in Unified Runtime Adapters here: oneapi-src/unified-runtime#934 Signed-off-by: Spruit, Neil R <[email protected]>

…ompile (#11464) piProgramBuild receives a list of devices, while urProgramBuild does not. This produces a series of issues when a UR program needs to be created for a specific device. So define a new API, called urProgramBuildExp to pass this list. Requires related patch in Unified Runtime Adapters here: oneapi-src/unified-runtime#934 --------- Signed-off-by: Spruit, Neil R <[email protected]> Co-authored-by: Kenneth Benzie (Benie) <[email protected]>

…ompile (intel#11464) piProgramBuild receives a list of devices, while urProgramBuild does not. This produces a series of issues when a UR program needs to be created for a specific device. So define a new API, called urProgramBuildExp to pass this list. Requires related patch in Unified Runtime Adapters here: oneapi-src/unified-runtime#934 --------- Signed-off-by: Spruit, Neil R <[email protected]> Co-authored-by: Kenneth Benzie (Benie) <[email protected]>

nrspruit mentioned this pull request Oct 7, 2023

[UR][L0] Add support for passing device list to urProgramBuild/Link/Compile intel/llvm#11464

Merged

nrspruit mentioned this pull request Oct 7, 2023

[UR][Adapters] Add support for passing device list to urProgramBuild/Link/Compile intel/llvm#11465

Closed

nrspruit force-pushed the fixprogrambuild_updated branch from 6e8d894 to 5f8335c Compare October 7, 2023 00:29

jandres742 reviewed Oct 7, 2023

View reviewed changes

source/adapters/cuda/program.cpp Outdated Show resolved Hide resolved

jandres742 reviewed Oct 7, 2023

View reviewed changes

source/adapters/hip/program.cpp Outdated Show resolved Hide resolved

nrspruit marked this pull request as draft October 9, 2023 06:39

nrspruit force-pushed the fixprogrambuild_updated branch from 5f8335c to 272406e Compare October 9, 2023 06:46

This was referenced Oct 10, 2023

[UR][L0] Add the multi-device-compile experimental feature #924

Merged

[UR][L0] Add support for passing device list to urProgramBuild #919

Merged

nrspruit marked this pull request as ready for review October 10, 2023 16:30

nrspruit requested a review from a team as a code owner October 10, 2023 16:30

kbenzie requested review from jchlanda and npmiller October 11, 2023 10:40

jchlanda approved these changes Oct 12, 2023

View reviewed changes

kbenzie reviewed Oct 12, 2023

View reviewed changes

kbenzie mentioned this pull request Oct 19, 2023

urProgramBuild doesnt take a list of devices, like piProgramBuild #912

Open

nrspruit force-pushed the fixprogrambuild_updated branch from cca8e98 to ca1829b Compare October 26, 2023 16:20

nrspruit requested review from a team as code owners October 26, 2023 16:20

mmoadeli reviewed Nov 13, 2023

View reviewed changes

aarongreig mentioned this pull request Nov 15, 2023

Cherry-pick multi device compile spec changes #1083

Closed

igchor and others added 5 commits November 16, 2023 17:22

Add exp-multi-device-compiler entry points to adapter interface loaders.

1bb0543

kbenzie force-pushed the fixprogrambuild_updated branch from 11ad98c to 26b9829 Compare November 16, 2023 17:28

kbenzie requested a review from a team as a code owner November 16, 2023 17:28

[L0] Fix urProgramLinkExp argument order

4b5e559

Align the `urProgramLinkExp` spec and implementation argument orders to fix Windows link error.

nrspruit force-pushed the fixprogrambuild_updated branch 5 times, most recently from 0f29db7 to 9c17051 Compare November 17, 2023 00:33

jandres742 reviewed Nov 17, 2023

View reviewed changes

kbenzie reviewed Nov 17, 2023

View reviewed changes

source/adapters/level_zero/program.cpp Outdated Show resolved Hide resolved

nrspruit force-pushed the fixprogrambuild_updated branch from 9c17051 to 20e8ee6 Compare November 17, 2023 15:56

Fix urProgramCompileExp, urProgramBuildExp, and urProgramLinkExp defi…

0790bf8

…nition to match spec Signed-off-by: Spruit, Neil R <[email protected]>

nrspruit force-pushed the fixprogrambuild_updated branch from 20e8ee6 to 0790bf8 Compare November 17, 2023 17:53

kbenzie merged commit ce152a6 into oneapi-src:adapters Nov 21, 2023

[UR][L0] Add support for passing device list to urProgramBuild/Link/Compile #934

[UR][L0] Add support for passing device list to urProgramBuild/Link/Compile #934

Uh oh!

Conversation

nrspruit commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jandres742 commented Oct 10, 2023

Uh oh!

jandres742 commented Oct 10, 2023

Uh oh!

kbenzie commented Oct 10, 2023

Uh oh!

jchlanda left a comment

Choose a reason for hiding this comment

Uh oh!

kbenzie left a comment

Choose a reason for hiding this comment

Uh oh!

nrspruit commented Nov 13, 2023

Uh oh!

mmoadeli Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jandres742 Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmoadeli Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbenzie commented Nov 14, 2023

Uh oh!

kbenzie commented Nov 15, 2023

Uh oh!

kbenzie commented Nov 16, 2023

Uh oh!

kbenzie commented Nov 16, 2023

Uh oh!

jandres742 commented Nov 16, 2023

Uh oh!

nrspruit commented Nov 17, 2023

Uh oh!

jandres742 Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

kbenzie Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nrspruit commented Oct 6, 2023 •

edited

Loading

mmoadeli Nov 13, 2023 •

edited

Loading

jandres742 Nov 14, 2023 •

edited

Loading

mmoadeli Nov 14, 2023 •

edited

Loading