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

[CUDA] Add support for binary type query #990

Closed

Conversation

fabiomestre
Copy link
Contributor

@fabiomestre fabiomestre commented Oct 23, 2023

CUDA does not make a distinction between binaryTypes (it treats PTX and binaries using the same entrypoints).

However, for UR, by definition:

  • urProgramCompile should set the binary type to UR_PROGRAM_BINARY_TYPE_COMPILED_OBJECT
  • urProgramBuild / urProgramLink should set it to UR_PROGRAM_BINARY_TYPE_EXECUTABLE.
  • urProgramCreateWithBinary should set the binary type to UR_PROGRAM_BINARY_TYPE_COMPILED_OBJECT

@fabiomestre fabiomestre force-pushed the fabio/add_binary_type_cuda branch 2 times, most recently from f4c6c37 to 385f706 Compare October 24, 2023 16:17
@fabiomestre fabiomestre marked this pull request as ready for review October 24, 2023 16:22
@fabiomestre fabiomestre requested a review from a team as a code owner October 24, 2023 16:22
@fabiomestre fabiomestre added the conformance Conformance test suite issues. label Nov 2, 2023
@fabiomestre
Copy link
Contributor Author

@oneapi-src/unified-runtime-cuda-write Would be great if someone could have a look a this PR

Copy link
Contributor

@jchlanda jchlanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Does it need any new test, or is it already covered?

/// Note: No calls to CUDA driver API in this function, only store binaries
/// for later.
///
/// Note: Only supports one device
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hdelan is that something we need to think about with multi device context (also the comment at the bottom of the diff)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be OK. The entry point has a Device param, so multi dev ctx just changes constructor of program to take the device as well

UR_RESULT_ERROR_INVALID_CONTEXT);
UR_ASSERT(size, UR_RESULT_ERROR_INVALID_SIZE);

ur_result_t Result = UR_RESULT_SUCCESS;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result doesn't seem to be needed, maybe wrap the calls directly in UR_ASSERT and return UR_SUCCESS at the end to simplify?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it to use UR_CHECK_ERROR 👍

@fabiomestre
Copy link
Contributor Author

LGTM

Does it need any new test, or is it already covered?

There is already a test in the UR CTS that checks this flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Conformance test suite issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants