Skip to content

[SYCL] Split spirv_ops.hpp into category headers#21849

Open
koparasy wants to merge 4 commits intointel:syclfrom
koparasy:sycl-split-spirv-ops-headers
Open

[SYCL] Split spirv_ops.hpp into category headers#21849
koparasy wants to merge 4 commits intointel:syclfrom
koparasy:sycl-split-spirv-ops-headers

Conversation

@koparasy
Copy link
Copy Markdown
Contributor

Refactor sycl/__spirv/spirv_ops.hpp into smaller category-specific headers
for atomic, image, subgroup, matrix, Intel math, and runtime declarations.

Keep spirv_ops.hpp as a compatibility umbrella, retarget direct consumers
to the narrower headers they actually use, and add the direct standard
includes needed to keep the new headers self-contained.

This results to compile time gains across several files.

@koparasy koparasy requested review from a team as code owners April 22, 2026 17:33
@koparasy koparasy requested review from againull and rafNNN April 22, 2026 17:33
Comment on lines +18 to +22
// TODO: drop these forward-declarations.
// As of now, compiler does not forward-declare long long overloads for
// these and as such we can't drop anything from here. But ideally, we should
// rely on the compiler to generate those - that would allow to drop
// spirv_ops.hpp include from more files.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. Implementing this TODO comment will reduce compile time much better than the header split.
  2. Why this header declares built-ins for the types other than long long? I expect https://github.com/intel/llvm/blob/sycl/clang/lib/Sema/SPIRVBuiltins.td#L1031-L1093 to automatically declare necessary functions for all the types except long long.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment and suggestion, that makes sense.

Just to clarify the scope of this PR: this change is intended as a split of the
existing declarations in spirv_ops.hpp into category-specific headers, not as
a functional change to the atomic declaration set itself. The primary benefit from this
PR is the avoidance of the inclusion of utility (std::pair) and some of the parsing/expansion cost.

The declarations currently in spirv_ops_atomic.hpp were moved over from the
existing umbrella header without changing their contents. For reference, they
come from the original spirv_ops.hpp

So, I agree, your point is complementary to this refactor relying on the
compiler-provided SPIR-V declarations for the non-long long cases and keeping
only the missing declarations would likely be a better compile-time improvement
for this area.

That looks like a worthwhile follow-up cleanup on top of this
header split.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's right. Instead of splitting spirv_ops.hpp, you can just remove all declarations which clang already provides. It's much better change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I want to be explicit about why I am still confused about the direction here.

The old umbrella spirv_ops.hpp costs about 45.8 ms to parse in the measurement
I took, and some of the heaviest transitive contributors on that path are exactly
the standard-library utility/type-traits machinery (30ms) that the split helps avoid
on narrower include paths.

So I agree your suggestion is a stronger optimization for the atomic slice
itself, but I do not understand it as making this split directionally wrong.
From my perspective, this PR still reduces unnecessary parsing for non-atomic
consumers, while your proposal reduces cost inside the atomic declarations.

Do you view this PR as moving in the wrong direction overall?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes. From my perspective we should remove declarations for ALL consumer, not just "non-atomic".
This will also remove the "the standard-library utility/type-traits machinery (30ms) that the split helps avoid
on narrower include paths".

Are you up to exploring this option?
If not, I'm not against making the split, I just saying that we can do much better that limiting the scope for __spirv_* functions declarations. I'll let @againull to decide.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I agree that removing the redundant declarations Clang already provides is a worthwhile direction, and it may well deliver a larger win for this area than the split alone.

My hesitation is only that I do not think it fully addresses all of the current utility / type_traits cost in spirv_ops.hpp.

For example, __spirv_IAddCarry and __spirv_ISubBorrow still use std::pair, and there are also type_traits-based helpers such as __spirv_ConvertPtrToU and the atomic min/max helper dispatch. So even after pruning the redundant atomic declarations, some broader refactoring would still be needed to remove those dependencies from all consumers.

I’m happy to explore that as follow-up work (later), but for this PR I’d prefer to keep the scope to the header split, since it is part of a larger refactoring series already in progress.

@againull let me know.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like removing all header declarations might require more effort/time, so I am not against the split.

Comment thread sycl/include/sycl/ext/oneapi/matrix/matrix-intel.hpp Outdated
@koparasy koparasy requested a review from dkhaldi April 23, 2026 17:59
Comment thread sycl/include/sycl/ext/oneapi/matrix/matrix-intel.hpp Outdated
Comment thread sycl/include/sycl/ext/intel/experimental/esimd/tfloat32.hpp
Copy link
Copy Markdown
Contributor

@dkhaldi dkhaldi left a comment

Choose a reason for hiding this comment

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

Joint matrix changes LGTM, thank you!

@koparasy
Copy link
Copy Markdown
Contributor Author

Hi @rafNNN , quick ping. Does this look OK to you,

@bratpiorka
Copy link
Copy Markdown
Contributor

Hi @rafNNN , quick ping. Does this look OK to you,

CUDA LGTM - approved

…aders

for atomic, image, subgroup, matrix, Intel math, and runtime declarations.

Keep spirv_ops.hpp as a compatibility umbrella, retarget direct consumers to
the narrower headers they actually use, and add the direct standard includes
needed to keep the new headers self-contained.
@koparasy koparasy force-pushed the sycl-split-spirv-ops-headers branch from 98d53ab to d885574 Compare April 27, 2026 19:50
@koparasy
Copy link
Copy Markdown
Contributor Author

@intel/dpcpp-esimd-reviewers can you check this out?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants