[SYCL] Add normalized -fsycl-id-queries-range= option#21706
[SYCL] Add normalized -fsycl-id-queries-range= option#21706againull wants to merge 4 commits intointel:syclfrom
-fsycl-id-queries-range= option#21706Conversation
Introduce -fsycl-id-queries-range={int,uint,none} option
to specify SYCL ID query range assumptions.
The flags -fsycl-id-queries-fit-in-int and
-fno-sycl-id-queries-fit-in-int are preserved as aliases:
- -fsycl-id-queries-fit-in-int → -fsycl-id-queries-range=int
- -fno-sycl-id-queries-fit-in-int → -fsycl-id-queries-range=none
Default behavior remains unchanged (range=int). If multiple options are
provided then last provided option wins.
The "uint" value will allow full unsigned range for SYCL ID queries (<=UNIT_MAX), which are
non-negative, providing similar benefits as -fsycl-id-queries-fit-in-int.
The reason why we don't simply change the behaviour of -fsycl-id-queries-fit-in-int
is because some users might already have the code like this:
int idx = index.get_global_id();
and changing the default assumption about ids to <= UINT_MAX can break
some optimizations.
Assisted-By: Claude Sonnet 4.5
|
I wonder if we should name the option |
That makes sense, let me update the PR. |
Remove tests that were using flags -fsycl-id-queries-fit-in-int and -fno-sycl-id-queries-fit-in-int with %clang_cc1. These flags are driver-level aliases that get translated to -fsycl-id-queries-range= before being passed to cc1, so they cannot be used directly with %clang_cc1.
There was a problem hiding this comment.
Pull request overview
This PR introduces a normalized SYCL compiler option -fsycl-id-queries-range={int,uint,size_t} to control ID query range assumptions, while preserving the legacy -f[no-]sycl-id-queries-fit-in-int flags as aliases. It threads the new setting through Clang options/LangOpts, predefined macros, SYCL headers’ assumption macro usage, NVPTX NVVM-reflect flags, documentation, and tests.
Changes:
- Add
-fsycl-id-queries-range=as an enum LangOpt with legacy flag aliases, and define new preprocessor macro__SYCL_ID_QUERIES_FIT_IN_UINT__when applicable. - Update SYCL headers to use a unified assumption macro
__SYCL_ASSUME_ID_RANGEand extend host-side launch-parameter validation to supportuintlimits. - Add/adjust driver, preprocessor, NVPTX, and SYCL E2E tests; update user and extension documentation.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sycl/test-e2e/Basic/range_offset_no_assumption.cpp | New E2E test for size_t (no additional assumption) and legacy alias behavior. |
| sycl/test-e2e/Basic/range_offset_fit_in_uint.cpp | New E2E test coverage for uint assumption mode. |
| sycl/test-e2e/Basic/range_offset_fit_in_int.cpp | Update to use normalized option and verify legacy alias remains functional; update expected message. |
| sycl/include/sycl/queue.hpp | Comment update to reflect generalized ID-range assumptions. |
| sycl/include/sycl/nd_item.hpp | Replace __SYCL_ASSUME_INT uses with __SYCL_ASSUME_ID_RANGE. |
| sycl/include/sycl/item.hpp | Replace __SYCL_ASSUME_INT uses with __SYCL_ASSUME_ID_RANGE. |
| sycl/include/sycl/id.hpp | Replace __SYCL_ASSUME_INT uses with __SYCL_ASSUME_ID_RANGE. |
| sycl/include/sycl/handler.hpp | Comment update to reflect generalized ID-range assumptions. |
| sycl/include/sycl/detail/id_queries_fit_in_int.hpp | Extend range/offset validation logic and messages to support both int and uint limits. |
| sycl/include/sycl/detail/defines.hpp | Introduce unified assumption macro __SYCL_ASSUME_ID_RANGE supporting int/uint and no-assumption modes. |
| sycl/include/sycl/detail/defines_elementary.hpp | Add default definition for __SYCL_ID_QUERIES_FIT_IN_UINT__. |
| sycl/doc/UsersManual.md | Document new normalized option and clarify legacy option equivalence. |
| sycl/doc/extensions/proposed/sycl_ext_oneapi_range_type.asciidoc | Update extension text to describe interaction with new -fsycl-id-queries-range= option. |
| libclc/libspirv/lib/ptx-nvidiacl/workitem/get_global_id.cl | Accept NVVM reflect flag for uint assumption in addition to int. |
| clang/test/Preprocessor/sycl-macro.cpp | Test new option values and the new predefined macro behavior. |
| clang/test/Driver/sycl.cpp | Validate driver forwarding/rewriting and “last option wins” precedence for normalized + legacy flags. |
| clang/test/Driver/sycl-specific-args-diagnostics.cpp | Add unused-arg warning coverage for -fsycl-id-queries-range= without -fsycl. |
| clang/test/Driver/sycl-nvptx-id-queries-fit-in-int.cpp | Extend NVPTX reflect-flag tests to cover int/uint/size_t and legacy aliases/precedence. |
| clang/lib/Frontend/InitPreprocessor.cpp | Define the correct predefined macro based on the new LangOpt enum. |
| clang/lib/Driver/ToolChains/Cuda.cpp | Emit appropriate NVVM reflect flags for int/uint range assumptions. |
| clang/lib/Driver/ToolChains/Clang.cpp | Forward only the normalized -fsycl-id-queries-range= to cc1 (aliases rewrite to it). |
| clang/include/clang/Options/Options.td | Add the normalized joined option + enum marshalling; implement legacy flags as aliases. |
| clang/include/clang/Basic/LangOptions.h | Introduce SYCLIdQueriesRangeKind enum. |
| clang/include/clang/Basic/LangOptions.def | Replace old bool langopt with enum langopt for SYCL ID query range assumptions. |
Replace problematic e2e tests that would launch kernels with huge ranges with proper unit tests that directly test the validation logic. The new unit tests: - Test all three modes (int, uint, size_t) by compiling the same test file three times with different preprocessor macro definitions - Use -D__SYCL_ID_QUERIES_FIT_IN_INT__=1 for INT mode - Use -D__SYCL_ID_QUERIES_FIT_IN_UINT__=1 for UINT mode - Use no macros for size_t mode (default when neither is defined) - Directly call sycl::detail::checkValueRange() to validate the logic without actually launching huge kernels - Test edge cases around INT_MAX and UINT_MAX limits - Test 1D, 2D, and 3D ranges, ids, and nd_ranges - Verify both success and failure paths with appropriate assertions
iclsrc
left a comment
There was a problem hiding this comment.
This PR cleanly replaces the boolean -f[no-]sycl-id-queries-fit-in-int option with a normalized -fsycl-id-queries-range={int,uint,size_t} enum option, while preserving backward compatibility via aliases. The implementation across driver, cc1, preprocessor macros, and runtime validation looks correct. One test infrastructure bug was found regarding preview test targets not receiving the required compile definitions.
| target_compile_definitions(IdQueriesRangeValidationInt_Non_Preview_Tests PRIVATE | ||
| __SYCL_ID_QUERIES_FIT_IN_INT__=1 | ||
| ) |
There was a problem hiding this comment.
🟡 Important: add_sycl_unittest creates both _Non_Preview_Tests and _Preview_Tests targets when SYCL_ENABLE_MAJOR_RELEASE_PREVIEW_LIB is enabled (see sycl/cmake/modules/AddSYCLUnitTest.cmake). Without applying compile definitions to the preview target, IdQueriesRangeValidationInt_Preview_Tests will compile without __SYCL_ID_QUERIES_FIT_IN_INT__=1 and silently fall into the #else (size_t) branch — running size_t mode tests instead of INT mode tests.
See the pattern used in sycl/unittests/compression/CMakeLists.txt and sycl/unittests/kernel-and-program/CMakeLists.txt.
| target_compile_definitions(IdQueriesRangeValidationInt_Non_Preview_Tests PRIVATE | |
| __SYCL_ID_QUERIES_FIT_IN_INT__=1 | |
| ) | |
| target_compile_definitions(IdQueriesRangeValidationInt_Non_Preview_Tests PRIVATE | |
| __SYCL_ID_QUERIES_FIT_IN_INT__=1 | |
| ) | |
| if(SYCL_ENABLE_MAJOR_RELEASE_PREVIEW_LIB) | |
| target_compile_definitions(IdQueriesRangeValidationInt_Preview_Tests PRIVATE | |
| __SYCL_ID_QUERIES_FIT_IN_INT__=1 | |
| __INTEL_PREVIEW_BREAKING_CHANGES | |
| ) | |
| endif() |
| target_compile_definitions(IdQueriesRangeValidationUInt_Non_Preview_Tests PRIVATE | ||
| __SYCL_ID_QUERIES_FIT_IN_UINT__=1 | ||
| ) |
There was a problem hiding this comment.
🟡 Important: Same issue as the INT block above — IdQueriesRangeValidationUInt_Preview_Tests will silently run as size_t mode tests when SYCL_ENABLE_MAJOR_RELEASE_PREVIEW_LIB is enabled.
| target_compile_definitions(IdQueriesRangeValidationUInt_Non_Preview_Tests PRIVATE | |
| __SYCL_ID_QUERIES_FIT_IN_UINT__=1 | |
| ) | |
| target_compile_definitions(IdQueriesRangeValidationUInt_Non_Preview_Tests PRIVATE | |
| __SYCL_ID_QUERIES_FIT_IN_UINT__=1 | |
| ) | |
| if(SYCL_ENABLE_MAJOR_RELEASE_PREVIEW_LIB) | |
| target_compile_definitions(IdQueriesRangeValidationUInt_Preview_Tests PRIVATE | |
| __SYCL_ID_QUERIES_FIT_IN_UINT__=1 | |
| __INTEL_PREVIEW_BREAKING_CHANGES | |
| ) | |
| endif() |
| TEST(IdQueriesRangeValidation, Int_Range1D_ExceedsLimit) { | ||
| // Skip if size_t can't hold values larger than INT_MAX | ||
| if (sizeof(size_t) <= sizeof(int)) { | ||
| GTEST_SKIP() << "size_t too small to test overflow beyond INT_MAX"; | ||
| } | ||
| range<1> r(static_cast<size_t>(INT_MAX) + 1); | ||
| EXPECT_THROW(detail::checkValueRange(r), exception); | ||
| } |
There was a problem hiding this comment.
🔵 Suggestion: The INT tests use a runtime if for the sizeof skip checks while the UINT tests below use if constexpr. Since these are compile-time constants, if constexpr is preferable — it avoids potential dead-code compiler warnings and is consistent with the UINT section. Consider changing the INT skip guards to if constexpr as well:
| TEST(IdQueriesRangeValidation, Int_Range1D_ExceedsLimit) { | |
| // Skip if size_t can't hold values larger than INT_MAX | |
| if (sizeof(size_t) <= sizeof(int)) { | |
| GTEST_SKIP() << "size_t too small to test overflow beyond INT_MAX"; | |
| } | |
| range<1> r(static_cast<size_t>(INT_MAX) + 1); | |
| EXPECT_THROW(detail::checkValueRange(r), exception); | |
| } | |
| TEST(IdQueriesRangeValidation, Int_Range1D_ExceedsLimit) { | |
| // Skip if size_t can't hold values larger than INT_MAX | |
| if constexpr (sizeof(size_t) <= sizeof(int)) { | |
| GTEST_SKIP() << "size_t too small to test overflow beyond INT_MAX"; | |
| } | |
| range<1> r(static_cast<size_t>(INT_MAX) + 1); | |
| EXPECT_THROW(detail::checkValueRange(r), exception); | |
| } |
| TEST(IdQueriesRangeValidation, Int_Id_ComponentExceedsLimit) { | ||
| // Skip if size_t can't hold values larger than INT_MAX | ||
| if (sizeof(size_t) <= sizeof(int)) { | ||
| GTEST_SKIP() << "size_t too small to test overflow beyond INT_MAX"; | ||
| } | ||
| id<3> offset(1, static_cast<size_t>(INT_MAX) + 1, 1); | ||
| EXPECT_THROW(detail::checkValueRange(offset), exception); | ||
| } |
There was a problem hiding this comment.
🔵 Suggestion: Same if vs if constexpr inconsistency as above.
| TEST(IdQueriesRangeValidation, Int_Id_ComponentExceedsLimit) { | |
| // Skip if size_t can't hold values larger than INT_MAX | |
| if (sizeof(size_t) <= sizeof(int)) { | |
| GTEST_SKIP() << "size_t too small to test overflow beyond INT_MAX"; | |
| } | |
| id<3> offset(1, static_cast<size_t>(INT_MAX) + 1, 1); | |
| EXPECT_THROW(detail::checkValueRange(offset), exception); | |
| } | |
| TEST(IdQueriesRangeValidation, Int_Id_ComponentExceedsLimit) { | |
| // Skip if size_t can't hold values larger than INT_MAX | |
| if constexpr (sizeof(size_t) <= sizeof(int)) { | |
| GTEST_SKIP() << "size_t too small to test overflow beyond INT_MAX"; | |
| } | |
| id<3> offset(1, static_cast<size_t>(INT_MAX) + 1, 1); | |
| EXPECT_THROW(detail::checkValueRange(offset), exception); | |
| } |
Introduce
-fsycl-id-queries-range={int,uint,size_t}option to specify SYCL ID query range assumptions.The flags
-fsycl-id-queries-fit-in-intand-fno-sycl-id-queries-fit-in-intare preserved as aliases:-fsycl-id-queries-fit-in-intcorresponds to-fsycl-id-queries-range=int-fno-sycl-id-queries-fit-in-intcorresponds to-fsycl-id-queries-range=size_tDefault behavior remains unchanged (
range=int). If multiple options are provided then last provided option wins.The
uintvalue will allow full unsigned range for SYCL ID queries (<=UINT_MAX), which are non-negative, providing similar benefits as-fsycl-id-queries-fit-in-int.The reason why we don't simply change the behavior of
-fsycl-id-queries-fit-in-intis because some users might already have the code like this:int idx = index.get_global_id();and changing the default assumption about ids to
<= UINT_MAXcan break some optimizations.Assisted-By: Claude Sonnet 4.5