[SYCL] Split spirv_ops.hpp into category headers#21849
[SYCL] Split spirv_ops.hpp into category headers#21849koparasy wants to merge 4 commits intointel:syclfrom
Conversation
| // 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. |
There was a problem hiding this comment.
- Implementing this TODO comment will reduce compile time much better than the header split.
- 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 exceptlong long.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's right. Instead of splitting spirv_ops.hpp, you can just remove all declarations which clang already provides. It's much better change.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It looks like removing all header declarations might require more effort/time, so I am not against the split.
dkhaldi
left a comment
There was a problem hiding this comment.
Joint matrix changes LGTM, thank you!
|
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.
98d53ab to
d885574
Compare
|
@intel/dpcpp-esimd-reviewers can you check this out? |
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.