[SYCL] Compile time/remove vec convert from builtins#21698
[SYCL] Compile time/remove vec convert from builtins#21698KornevNikita merged 4 commits intointel:syclfrom
Conversation
046824f to
ea1856e
Compare
Stop pulling vec::convert in through SYCL builtin headers by using a local relational mask widening path and a public sycl/vector_convert.hpp wrapper for opt-in conversion support. This reduces header dependencies and improves compile time by about 9.3% for host compilation and 7.5% for device compilation on simple translation unit.
ea1856e to
b6367fd
Compare
YuriPlyakhin
left a comment
There was a problem hiding this comment.
change in sycl/test-e2e/BFloat16/bfloat16_vec.cpp LGTM
|
Hi @uditagarwal97 , just a quick ping on this PR when you get a chance. |
There was a problem hiding this comment.
Pull request overview
This PR reduces compile-time header dependencies by stopping SYCL builtin headers from pulling in vec::convert implicitly, and instead introducing a public opt-in wrapper header (<sycl/vector_convert.hpp>) plus a local mask-widening path for relational builtins.
Changes:
- Add public
<sycl/vector_convert.hpp>wrapper and include-dependency test coverage for it. - Remove implicit
detail/vector_convert.hppdependency from builtin/khr include chains; update include-deps tests accordingly. - Replace relational builtin mask widening from
vec::convertto a localrelational_mask_widenimplementation; update device-code checks.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sycl/include/sycl/detail/builtins/builtins.hpp | Drops detail/vector_convert.hpp, adds relational_mask_widen helper to avoid pulling vec conversion machinery into builtins. |
| sycl/include/sycl/detail/builtins/relational_functions.inc | Uses relational_mask_widen instead of vec::convert for relational mask widening. |
| sycl/include/sycl/vector_convert.hpp | New public wrapper header for opt-in vec::convert support. |
| sycl/include/sycl/sycl.hpp | Switches umbrella header to include the new public vector_convert.hpp rather than the detail header. |
| sycl/include/sycl/detail/image_accessor_util.hpp | Updates includes to pull in vec::convert support via the new public header. |
| sycl/include/sycl/ext/oneapi/experimental/bfloat16_math.hpp | Updates include to use public sycl/vector_convert.hpp. |
| sycl/include/sycl/ext/oneapi/bf16_storage_builtins.hpp | Adds missing exception header dependency for host-side throws. |
| sycl/source/detail/image_accessor_util.cpp | Updates include to use public sycl/vector_convert.hpp. |
| sycl/test/include_deps/sycl_vector_convert.hpp.cpp | New include-dependency test for the public vector_convert.hpp header. |
| sycl/test/include_deps/sycl_khr_includes_usm.hpp.cpp | Updates expected include dependency list after removing implicit vector-convert pull-in. |
| sycl/test/include_deps/sycl_khr_includes_math.hpp.cpp | Updates expected include dependency list after removing implicit vector-convert pull-in. |
| sycl/test/include_deps/sycl_khr_includes_stream.hpp.cpp | Updates expected include dependency list after removing implicit vector-convert pull-in. |
| sycl/test/include_deps/sycl_khr_includes_reduction.hpp.cpp | Updates expected include dependency list after removing implicit vector-convert pull-in. |
| sycl/test/check_device_code/vector/bf16_builtins_old_vec.cpp | Adjusts LLVM IR checks to match new mask widening/conversion lowering. |
| sycl/test/check_device_code/vector/bf16_builtins_new_vec.cpp | Adjusts LLVM IR checks to match new mask widening/conversion lowering. |
| sycl/test-e2e/Basic/vector/int-convert.cpp | Switches from detail include to public sycl/vector_convert.hpp. |
| sycl/test-e2e/Basic/vector/byte.cpp | Switches from detail include to public sycl/vector_convert.hpp. |
| sycl/test-e2e/Basic/vector/bool.cpp | Switches from detail include to public sycl/vector_convert.hpp. |
| sycl/test-e2e/Basic/sycl_2020_images/common.hpp | Switches from detail include to public sycl/vector_convert.hpp. |
| sycl/test-e2e/Basic/half_builtins.cpp | Adds explicit include of public sycl/vector_convert.hpp. |
| sycl/test-e2e/Basic/char_builtins.cpp | Adds explicit include of public sycl/vector_convert.hpp. |
| sycl/test-e2e/BFloat16/bfloat16_vec.cpp | Switches from detail include to public sycl/vector_convert.hpp. |
uditagarwal97
left a comment
There was a problem hiding this comment.
Overall, I support the idea of decoupling builtins from the heavy vec::convert, if it helps improving compilation time.
My concern is with the new sycl/vector_convert.hpp header - I think that's unnecessary as users can just include sycl/detail/vector_convert.hpp header. @cperkinsintel WDYT?
Good point. I agree users can include the detail/ header today. My intention with introducing That said, I agree this is not strictly required for the compile-time improvement in this PR. If we prefer to keep the scope minimal, I’m happy to drop the public header here and follow up separately with a more explicit discussion on whether we want to expose this as part of the public API. |
Yes, I think it would be better to drop public header change from this PR. |
|
There are other changes to vec being discussed, if there is going to be one or more public headers for it, we should decide that later. |
|
@uditagarwal97 I removed the header and updated tests etc. |
|
@intel/llvm-gatekeepers please consider merging |
Stop pulling
vec::convertin through SYCL builtin headers by using a local relational mask widening path and a publicsycl/vector_convert.hppwrapper for opt-in conversion support.This reduces header dependencies and improves compile time by about 9.3% for host compilation and 7.5% for device compilation when including
<sycl/builtin.hpp>