[libcu++] Optimize cuda::std::saturating_cast#9724
Conversation
|
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 (8)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR tightens ChangesSaturating cast device implementation and tests
Suggested reviewers: Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libcudacxx/include/cuda/std/__numeric/saturating_cast.h (1)
34-337: 📐 Maintainability & Code Quality | 🔵 Trivialsuggestion: this branch matrix (4 size classes × up to 4 sign combos) is highly repetitive — same shape of
if constexpr+asm(...)+ assignment repeated ~30 times. Consider a small helper macro parameterized by destination/source PTX type suffixes and register constraints to cut duplication, if consistent with how similar per-width/per-sign PTX dispatch is done elsewhere in the codebase.libcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturating_cast.pass.cpp (1)
22-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: add a device-side call site here, mirroring
saturating_div.pass.cpp, so theNV_IS_DEVICEbranch incuda::std::saturating_castis actually exercised. Right now this file only covers host/static-evaluation paths, leaving__saturating_cast_impl_deviceuntested.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1a0bf500-19c2-4d07-8bf4-b4fa47f94015
📒 Files selected for processing (8)
libcudacxx/include/cuda/__numeric/saturating_overflow_cast.hlibcudacxx/include/cuda/std/__numeric/saturating_cast.hlibcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturate_cast.pass.cpplibcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturating_add.pass.cpplibcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturating_cast.pass.cpplibcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturating_div.pass.cpplibcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturating_mul.pass.cpplibcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturating_sub.pass.cpp
💤 Files with no reviewable changes (1)
- libcudacxx/test/libcudacxx/std/numerics/numeric.ops/numeric.ops.sat/saturate_cast.pass.cpp
6a8d7c5 to
2b4d39f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2b4d39f to
a23b125
Compare
fbusato
left a comment
There was a problem hiding this comment.
looks good. Only one comment on int128
|
|
||
| #if _CCCL_CUDA_COMPILATION() | ||
| _CCCL_TEMPLATE(class _Up, class _Tp) | ||
| _CCCL_REQUIRES((sizeof(_Tp) == sizeof(int8_t))) |
There was a problem hiding this comment.
Did you mean sizeof(_Up) here (and the other functions)? The body checks sizeof(_Tp)
There was a problem hiding this comment.
Omg yes, thank you!
😬 CI Workflow Results🟥 Finished in 2h 09m: Pass: 50%/120 | Total: 2d 12h | Max: 2h 09m | Hits: 92%/132453See results here. |
We can use inline PTX to make
ptxasgenerate a singleI2Iinstruction in most of the cases.I've also renamed
{op}_sattests tosaturating_{op}, I forgot to do that when renaming those functions.