Skip to content

src: remove unnecessary compute device info header dependencies #3428

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

rjoursler
Copy link
Contributor

@rjoursler rjoursler commented Jun 16, 2025

The nGEN interface used for devices is ngen::Product. To enable better interoperation between oneDNN and nGEN, this PR modifies device_info_t to store the ngen::Product. In particular, as intel GPU third party dependencies are included via src/gpu/CMakeLists.txt, nGEN is not available outside this directory, so implicit dependencies on gpu/intel/compute/device_info.hpp outside of the src/gpu/ directories are removed. In particular, these implicit dependencies were injected via gpu/intel/compute_engine.hpp, gpu/intel/compute_kernel.hpp, and gpu/intel/ocl/ocl_utils.hpp. This PR restructures these headers to remove the device_info.hpp dependency from these headers.

@rjoursler rjoursler requested review from a team as code owners June 16, 2025 15:17
@github-actions github-actions bot added platform:gpu-intel Codeowner: @oneapi-src/onednn-gpu-intel component:graph-api Codeowner: @oneapi-src/onednn-graph component:common third_party labels Jun 16, 2025
@rjoursler rjoursler force-pushed the rjoursle/break_dependency branch from 301089a to b661642 Compare June 16, 2025 15:49
Copy link
Contributor

@dzarukin dzarukin left a comment

Choose a reason for hiding this comment

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

Could you, please, extend the PR description with overview changes and what's the actual benefit of adding a direct nGEN dependency and a bunch of forward declaration to support it? Having an explanation why direct use of nGEN header is not possible given that a direct dependency is introduced would be helpful as well.

@rjoursler
Copy link
Contributor Author

Having an explanation why direct use of nGEN header is not possible given that a direct dependency is introduced would be helpful as well.

I added a description. The core issue is that the nGEN headers are only enabled within the src/gpu directory, so we get compilation errors if we add an nGEN dependency to device_info.hpp. We could alternatively just include nGEN headers everywhere in oneDNN (when applicable), but this PR keeps the current status quo as nGEN really should not be necessary as a dependency in src/xpu or src/graph,

@rjoursler rjoursler force-pushed the rjoursle/remove_unused branch from 95bb066 to 221f55c Compare June 18, 2025 15:08
@dzarukin
Copy link
Contributor

@spalicki, Could you, please, take a look one this one?

Base automatically changed from rjoursle/remove_unused to main June 19, 2025 15:57
@dzarukin dzarukin requested a review from spalicki June 22, 2025 21:04
@rjoursler rjoursler force-pushed the rjoursle/break_dependency branch 3 times, most recently from e1572a1 to 1892567 Compare July 1, 2025 21:02
@rjoursler rjoursler force-pushed the rjoursle/break_dependency branch from 1892567 to 1cbd902 Compare July 2, 2025 00:57
@rjoursler rjoursler requested a review from a team as a code owner July 2, 2025 00:57
@github-actions github-actions bot added the platform:gpu-generic Codeowner: @oneapi-src/onednn-gpu-generic label Jul 2, 2025
@rjoursler rjoursler force-pushed the rjoursle/break_dependency branch 3 times, most recently from 68132d7 to 5d27e35 Compare July 2, 2025 12:47
@rjoursler
Copy link
Contributor Author

make test
disable test_device_cpu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:common component:graph-api Codeowner: @oneapi-src/onednn-graph platform:gpu-generic Codeowner: @oneapi-src/onednn-gpu-generic platform:gpu-intel Codeowner: @oneapi-src/onednn-gpu-intel third_party
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants