Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jandres742 opened this issue Oct 1, 2023 · 8 comments
Open
Assignees
Labels
specification Changes or additions to the specification

Comments

@jandres742
Copy link

jandres742 commented Oct 1, 2023

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 we need to define a new API, something like urProgramBuildExp to pass this list (please see here #911)

Another alternative is to add numDevices and phDevices to existing urProgramBuild:

  • if numDevices is zero, the urProgramBuild retains current behavior, where program is build for all devices in the context
  • if numDevices is non-zero, then urProgramBuild builds only for those devices.

Other suggestions are welcome.

@kbenzie
Copy link
Contributor

kbenzie commented Oct 2, 2023

Leaving out the device list was a deliberate decision as we were under the impression that compiling for all devices in the context would be sufficient.

Is the issue specificatlly with the mismatch between piProgramBuild and urProgramBuild or is compiling for a specific device, rather than all devices in the context, a use case we need to support moving forwards?

@kbenzie kbenzie added needs-discussion This needs further discussion specification Changes or additions to the specification labels Oct 2, 2023
@kbenzie
Copy link
Contributor

kbenzie commented Oct 9, 2023

#919 and #924 address the specification, will need to follow up PR's the implementation in adpaters. It has been noted that some adapters may not be able to support device-lists as specified in these entry-points.

@kbenzie kbenzie self-assigned this Oct 9, 2023
@jandres742
Copy link
Author

#919 and #924 address the specification, will need to follow up PR's the implementation in adpaters. It has been noted that some adapters may not be able to support device-lists as specified in these entry-points.

Thanks @kbenzie . I think if a given adapter cannot support multi-device kernel compilation then they can return UNSUPPORTED.

However, the PI interfaces already accept a list of devices, so actually adding that allows us to keep compatibility with PI.

Is the issue specificatlly with the mismatch between piProgramBuild and urProgramBuild or is compiling for a specific device, rather than all devices in the context, a use case we need to support moving forwards?

it is specifically with the mismatch. In a multi-device scenario, piProgramBuild is called several times, with only 1 device, allowing L0 to compile the program for that device, one at a time.

However, w/o having the list, L0 doesnt know which device to compile in specifically, it could compile for all devices as you mention, but that support is not there yet (to be added in #942), so by not having the list of devices, behavior was broken with respect to behavior with PI.

@jandres742
Copy link
Author

some feedback from others making a case for these APIs to be promoted to standard:

  • what about if application wants to compile differently for each device?
  • what about if application wants to compile just for a subset of devices in the context?

@kbenzie
Copy link
Contributor

kbenzie commented Oct 19, 2023

I've created v0.7.2 after merging the same spec changes used in the multi-device-compile-hotfix branch to v0.7.x.

I'll aim to merge #924 to main shortly to ensure we have consistent availability of the experimental feature. From there we need to get the main and adapters branches in sync so we can merge the implementation in #934.

Once all those steps are completed, we can make a final decision on promoting the experimental feature to core.

@kbenzie
Copy link
Contributor

kbenzie commented Oct 27, 2023

#924 is now merged to main. @omarahmed1111 is working on getting the adapters branch up to date with main. We will then merge #934 after.

@kbenzie
Copy link
Contributor

kbenzie commented Nov 22, 2023

We have now merged the spec and implementation changes to the adapters branch in #934 which is also now merged in intel/llvm#11464. This is functionally complete from the point of view of the experimental feature but I'll leave this open until we've decided about making these part of the core spec.

@kbenzie kbenzie removed the needs-discussion This needs further discussion label Jan 10, 2024
@aarongreig
Copy link
Contributor

addressed by #1195

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
specification Changes or additions to the specification
Projects
None yet
Development

No branches or pull requests

3 participants