[libcu++] Fix __is_sequence definition for arguments framework#9271
[libcu++] Fix __is_sequence definition for arguments framework#9271miscco wants to merge 1 commit into
__is_sequence definition for arguments framework#9271Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
OverviewThis PR fixes the ChangesImplementation (
|
| Layer / File(s) | Summary |
|---|---|
Core implementation: sequence and traits reclassification libcudacxx/include/cuda/__argument/argument.h |
Adds range/type-trait includes; redefines __is_sequence_v<_Tp> as `::cuda::std::is_array_v<::cuda::std::remove_cvref_t<_Tp>> |
Test validation of classification changes libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp, libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp, libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp |
Updates test includes and static_assert expectations: int*, counting_iterator, and wrapper *_like types are now non-sequences; arrays and span/array remain sequences; inverts several is_single_value checks to use !__is_sequence_v instead of the removed predicate. |
-
Possibly related PRs:
- NVIDIA/cccl#8875: Updates
argument.hsequence/single-value trait logic similar to this PR. - NVIDIA/cccl#9074: Refactors consumers to rely on
::cuda::__argument::__traits<>affected by these trait changes.
- NVIDIA/cccl#8875: Updates
-
Suggested reviewers:
- ericniebler
- Jacobfaib
- gevtushenko
Warning
There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.
🔧 Infer (1.2.0)
libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp
libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found
11 | #include <cuda/argument>
| ^~~~~~~~~~~~~~~~~~
1 error generated.
Error: the following clang command did not run successfully:
/opt/infer-linux-x86_64-v1.2.0/lib/infer/facebook-clang-plugins/clang/install/bin/clang-18
@/tmp/coderabbit-infer/f13ff250a89da6bae7e336f74c8f3f19aecde1fb-07842b3345a172fc/tmp/clang_command_.tmp.a0d1ef.txt
++Contents of '/tmp/coderabbit-infer/f13ff250a89da6bae7e336f74c8f3f19aecde1fb-07842b3345a172fc/tmp/clang_command.tmp.a0d1ef.txt':
"-cc1" "-load"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../../facebook-clang-plugins/libtooling/build/FacebookClangPlugin.dylib"
"-add-plugin" "BiniouASTExporter" "-plugin-arg-BiniouASTExporter" "-"
"-plugin-arg-BiniouASTExporter" "PREPEND_CURRENT_DIR=1"
"-plugin-arg-BiniouASTExporter" "MAX_STRING_SIZE=65535" "-cc1" "-triple"
"x86_64-unknown-li
... [truncated 1169 characters] ...
al-isystem" "/usr/local/include" "-internal-isystem"
"/usr/lib/gcc/x86_64-linux-gnu/12/../../../../x86_64-linux-gnu/include"
"-internal-externc-isystem" "/usr/include/x86_64-linux-gnu"
"-internal-externc-isystem" "/include" "-internal-externc-isystem"
"/usr/include" "-Wno-ignored-optimization-argument" "-Wno-everything"
"-fdeprecated-macro" "-ferror-limit" "19" "-fgnuc-version=4.2.1"
"-fskip-odr-check-in-gmf" "-fcxx-exceptions" "-fexceptions"
"-D__GCC_HAVE_DWARF2_CFI_ASM=1" "-o"
"/tmp/coderabbit-infer/07842b3345a172fc/file.o" "-x" "c++"
"libcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpp" "-O0"
"-fno-builtin" "-include"
"/opt/infer-linux-x86_64-v1.2.0/lib/infer/infer/bin/../lib/clang_wrappers/global_defines.h"
"-Wno-everything"
libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp
libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found
11 | #include <cuda/_argument>
| ^~~~~~~~~~~~~~~~~~
1 error generated.
libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp:161:3-166:3: ERROR translating statement 'CompoundStmt'
Aborting translation of method 'test' in file 'libcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.ml", line 658, characters 12-59
Called from ClangFrontend__CTrans.CTrans_funct.destructor_calls.(fun) in file "src/clang/cTrans.ml", line 2048, characters 12-69
Called from Base__List.rev_filter
... [truncated 2200 characters] ...
from ClangFrontend__CTrans.CTrans_funct.exec_with_node_creation in file "src/clang/cTrans.ml" (inlined), line 104, characters 20-38
Called from ClangFrontend__CTrans.CTrans_funct.get_clang_stmt_trans in file "src/clang/cTrans.ml" (inlined), line 5395, characters 4-69
Called from ClangFrontend__CTrans.CTrans_funct.get_custom_stmt_trans in file "src/clang/cTrans.ml", line 5401, characters 8-55
Called from ClangFrontend__CTrans.CTrans_funct.exec_trans_instrs.exec_trans_instrs_rev in file "src/clang/cTrans.ml" (inlined), line 5365, characters 28-54
Called from ClangFrontend__CTrans.CTrans_funct.exec_trans_instrs in file "src/clang/cTrans.ml" (inlined), line 5389, characters 6-70
Called from ClangFrontend__CTrans.CTrans_funct.instructions_trans in file "src/clang/cTrans.ml", line 5451, characte
libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp:11:10: fatal error: 'cuda/_argument' file not found
11 | #include <cuda/_argument>
| ^~~~~~~~~~~~~~~~~~
1 error generated.
libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp:124:3-128:3: ERROR translating statement 'CompoundStmt'
Aborting translation of method 'test' in file 'libcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp': "Assert_failure src/clang/cAst_utils.ml:249:53"
Uncaught Internal Error: "Assert_failure src/clang/cAst_utils.ml:249:53"
Error backtrace:
Raised at ClangFrontend__CAst_utils.get_decl_from_typ_ptr in file "src/clang/cAst_utils.ml", line 249, characters 53-65
Called from ClangFrontend__CTrans.CTrans_funct.get_destructor_decl_ref in file "src/clang/cTrans.ml", line 658, characters 12-59
Called from ClangFrontend__CTrans.CTrans_funct.destructor_calls.(fun) in file "src/clang/cTrans.ml", line 2048, characters 12-69
Called from Base__List.rev_filter_ma
... [truncated 2200 characters] ...
Frontend__CTrans.CTrans_funct.get_clang_stmt_trans in file "src/clang/cTrans.ml" (inlined), line 5395, characters 4-69
Called from ClangFrontend__CTrans.CTrans_funct.get_custom_stmt_trans in file "src/clang/cTrans.ml", line 5401, characters 8-55
Called from ClangFrontend__CTrans.CTrans_funct.exec_trans_instrs.exec_trans_instrs_rev in file "src/clang/cTrans.ml" (inlined), line 5365, characters 28-54
Called from ClangFrontend__CTrans.CTrans_funct.exec_trans_instrs in file "src/clang/cTrans.ml" (inlined), line 5389, characters 6-70
Called from ClangFrontend__CTrans.CTrans_funct.instructions_trans in file "src/clang/cTrans.ml", line 5451, characters 25-68
Called from ClangFrontend__CFrontend_decl.CFrontend_decl_funct.add_method.f in file "src/clang/cFrontend_decl.ml", line 48, characters 12-91
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libcudacxx/include/cuda/__argument/argument.h (1)
114-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winimportant: The static_assert error messages "must have a distinct element type" are now incorrect. The new
__is_sequence_vchecks whether a type is an array or satisfiesranges::range, not whether it has a distinct element type. Types likeelement_type_like<int>have a distinct element type but are not arrays or ranges, so they would fail this check for a different reason than the message suggests.Update the messages to reflect the actual requirement, e.g., "must be an array or range type".
Proposed fix
- static_assert(__is_sequence_v<value_type>, "constant sequence arguments must have a distinct element type"); + static_assert(__is_sequence_v<value_type>, "constant sequence arguments must be an array or range type");Apply similar changes to lines 304, 701, and 714.
Also applies to: 304-304, 701-701, 714-714
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6b49057a-ad0c-4843-8771-66b0fd1e7ae9
📒 Files selected for processing (4)
libcudacxx/include/cuda/__argument/argument.hlibcudacxx/test/libcudacxx/cuda/argument/argument_traits.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/dynamic_argument.pass.cpplibcudacxx/test/libcudacxx/cuda/argument/static_argument.pass.cpp
miscco
force-pushed
the
fix_argument_sequence_definition
branch
from
171cb93 to
90a8c02
Compare
The current is_sequence definition is thoroughly broken. An iterator is not a sequence and neither is a pointer. The main problem here is that this precludes passing along iterators / pointers as values which is a reasonable use case. It also completely deviates from any standard handling in C++. Rather than reinventing the wheel in a bad way use standard terminology to determine what is a range and what not. A user can always wrap iterators and pointers into views and spans, so there is not need for the current definition
miscco
force-pushed
the
fix_argument_sequence_definition
branch
from
90a8c02 to
f13ff25
Compare
|
I have added some more tests to make sure this does not regresses again. I believe we can agree that |
😬 CI Workflow Results🟥 Finished in 2h 01m: Pass: 91%/115 | Total: 19h 55m | Max: 1h 53m | Hits: 98%/338419See results here. |
| inline constexpr bool __is_iterable_v<_Tp, | ||
| ::cuda::std::void_t<decltype(::cuda::std::declval<const _Tp&>().begin()), | ||
| decltype(::cuda::std::declval<const _Tp&>().end())>> = true; | ||
| ::cuda::std::is_array_v<::cuda::std::remove_cvref_t<_Tp>> || ::cuda::std::ranges::range<_Tp>; |
There was a problem hiding this comment.
An array is a range though, no? Why do you need both conditionals?
|
I am not tied to the name of this trait, but at the end of the day when user passes something that has bounds we need to understand if its one value or multiple values. We have the I highly doubt we can convince others CUB should stop accepting iterators and pointers, so I believe we need a broader definition than just range, but maybe we can also remove the extra validation, which would remove the need for this trait. But it is likely in that case a similar trait would need to live in CUB anyway, so I am not sure if it isn't just pushing the problem to a different place |
3 participants
The current is_sequence definition is thoroughly broken. An iterator is not a sequence and neither is a pointer.
The main problem here is that this precludes passing along iterators / pointers as values which is a reasonable use case.
It also completely deviates from any standard handling in C++.
Rather than reinventing the wheel in a bad way use standard terminology to determine what is a range and what not.
A user can always wrap iterators and pointers into views and spans, so there is not need for the current definition