Skip to content

Report device fp support via config rather than extension string. #2231

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Report device fp support via config rather than extension string. #2231

wants to merge 1 commit into from

Conversation

aarongreig
Copy link
Contributor

@aarongreig aarongreig commented Oct 22, 2024 •
edited
Loading

We're trying to move the UR adapters away from returning hard coded OpenCL
extension strings to report device capabilities, this is the first change
in that direction.

See #1374

@aarongreig aarongreig requested review from a team as code owners October 22, 2024 14:24
@aarongreig aarongreig requested a review from npmiller October 22, 2024 14:24
@github-actions github-actions bot added level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues labels Oct 22, 2024
frasercrmck
frasercrmck reviewed Oct 22, 2024
View reviewed changes
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

I might just be out of the loop but I'm not sure why this is being done. I'm not sure how to review it if I don't understand the rationale.

@aarongreig aarongreig force-pushed the aaron/stopReportingFPExtensions branch from 5c227f9 to c740185 Compare October 22, 2024 14:35
@aarongreig
Copy link
Contributor Author

My bad, updated the commit message and description. Basically I think returning hard coded opencl extension strings from every adapter instead of reporting stuff via device info queries is heinous and I'm trying get rid of it.

@aarongreig
Copy link
Contributor Author

llvm PR intel/llvm#15811

@aarongreig aarongreig force-pushed the aaron/stopReportingFPExtensions branch 4 times, most recently from 88ef7a8 to 5d3b502 Compare October 29, 2024 14:07
@aarongreig aarongreig force-pushed the aaron/stopReportingFPExtensions branch from 5d3b502 to febe542 Compare October 29, 2024 17:33
@aarongreig aarongreig requested a review from a team as a code owner October 29, 2024 17:33
@github-actions github-actions bot added the native-cpu Native CPU adapter specific issues label Oct 29, 2024
@aarongreig
Copy link
Contributor Author

ping @oneapi-src/unified-runtime-native-cpu-write @oneapi-src/unified-runtime-opencl-write @oneapi-src/unified-runtime-level-zero-write @oneapi-src/unified-runtime-cuda-write @oneapi-src/unified-runtime-hip-write

nrspruit
nrspruit approved these changes Nov 15, 2024
View reviewed changes
Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

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

LGTM for L0

@aarongreig aarongreig force-pushed the aaron/stopReportingFPExtensions branch from febe542 to b982b09 Compare January 22, 2025 15:40
@aarongreig
Copy link
Contributor Author

ping @oneapi-src/unified-runtime-native-cpu-write @oneapi-src/unified-runtime-opencl-write @oneapi-src/unified-runtime-cuda-write @oneapi-src/unified-runtime-hip-write

frasercrmck
frasercrmck approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@frasercrmck frasercrmck left a comment

Choose a reason for hiding this comment

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

CUDA/HIP LGTM

@aarongreig aarongreig force-pushed the aaron/stopReportingFPExtensions branch from b982b09 to fabf436 Compare February 4, 2025 15:54
coldav
coldav reviewed Feb 5, 2025
View reviewed changes
source/adapters/native_cpu/device.cpp
case UR_DEVICE_INFO_DOUBLE_FP_CONFIG: {
ur_device_fp_capability_flags_t DoubleFPValue = 0;
return ReturnValue(DoubleFPValue);
// All fp types are supported, return minimum flags to indicate support.
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we falsely say before we didn't support double etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no the adapter was reporting "cl_khr_fp16, cl_khr_fp64 " for UR_DEVICE_INFO_EXTENSIONS, which I've removed to replace with this

Copy link
Contributor

Choose a reason for hiding this comment

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

so this bit didn't get called for the extensions then, but was still wrong?

coldav
coldav reviewed Feb 5, 2025
View reviewed changes
source/adapters/native_cpu/device.cpp
ur_device_fp_capability_flags_t SingleFPValue = 0;
return ReturnValue(SingleFPValue);
}
case UR_DEVICE_INFO_HALF_FP_CONFIG:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think half support will depend on the architecture - although x86 seems to emulate - still this may still be better than what we had before, and we don't officially support the "bad" architectures I think yet.

@aarongreig aarongreig added the ready to merge Added to PR's which are ready to merge label Feb 6, 2025
@martygrant
Copy link
Contributor

Unified Runtime -> intel/llvm Repo Move Notice

Information

The source code of Unified Runtime has been moved to intel/llvm under the unified-runtime top-level directory,
all future development will now be carried out there. This was done in intel/llvm#17043.

The code will be mirrored to oneapi-src/unified-runtime and the specification will continue to be hosted at oneapi-src.github.io/unified-runtime.

The contribution guide has been updated with new instructions for contributing to Unified Runtime.

PR Migration

All open PRs including this one will be labelled auto-close and shall be automatically closed after 30 days.
To allow for some breathing space, this automation will not be enabled until next week (27/02/2025).

Should you wish to continue with your PR you will need to migrate it to intel/llvm.
We have provided a script to help automate this process.

This is an automated comment.

@aarongreig
Report device fp support via config rather than extension string.
94ea653 
We're trying to move the UR adapters away from returning hard coded OpenCL
extension strings to report device capabilities, this is the first change
in that direction.
@aarongreig aarongreig force-pushed the aaron/stopReportingFPExtensions branch from fabf436 to 94ea653 Compare February 24, 2025 15:49
@aarongreig aarongreig mentioned this pull request Feb 24, 2025
Open
@aarongreig aarongreig closed this Feb 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frasercrmck frasercrmck frasercrmck approved these changes

@kbenzie kbenzie kbenzie approved these changes

@nrspruit nrspruit nrspruit approved these changes

@coldav coldav coldav approved these changes

@npmiller npmiller Awaiting requested review from npmiller npmiller was automatically assigned from oneapi-src/unified-runtime-hip-write

Assignees
No one assigned
Labels
cuda CUDA adapter specific issues hip HIP adapter specific issues level-zero L0 adapter specific issues native-cpu Native CPU adapter specific issues ready to merge Added to PR's which are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@aarongreig @martygrant @frasercrmck @kbenzie @nrspruit @coldav