Test for corner-cases in mul_overflow.pass.cpp#9792
Conversation
|
@fbusato if you agree with the approach, can you run |
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesMultiply overflow test coverage
Assessment against linked issues
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
libcudacxx/test/libcudacxx/cuda/numeric/overflow.arithmetic/mul_overflow.pass.cpp (2)
110-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Add an explicit fixed-width-integer header for the new aliases and use the
cuda::stdaliases required by this test suite. These lines currently rely on transitive exposure ofint8_t,int32_t,int64_t,uint32_t, anduint64_t. As per coding guidelines, include all headers needed by used symbols and do not rely on transitive includes.Source: Coding guidelines
133-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: Declare
uint128_maxasconstexpr; its initializer is compile-time evaluable. As per coding guidelines, compile-time-evaluable variables must beconstexpr.Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e1e97b45-bf14-4c29-9e0a-f5bb7f5ca52e
📒 Files selected for processing (1)
libcudacxx/test/libcudacxx/cuda/numeric/overflow.arithmetic/mul_overflow.pass.cpp
|
/ok to test ece8939 |
😬 CI Workflow Results🟥 Finished in 2h 11m: Pass: 97%/71 | Total: 2d 01h | Max: 1h 59m | Hits: 99%/328843See results here. |
|
Although the test failures themselves do not seem to be related to this PR, I actually found that there is a slowdown! This is from libcu++ nvcc GCC / [CTK13.3 GCC15 C++17] Build(amd64) log: However, with other runs I have noticed that the slowest tests very quite a bit. Here are two random ones I pulled up: |
|
Great news; I am finding that this behavior might actually be completely expected, and that the kernels are being cached. I can deterministically get the slowdown by making sure the compiler has not come across the With that being said, merging this now would make the |
|
smaller PRs are always better. My current concern is that the new test cases are hardcode for specific types. Would be possible to generalize them? e.g. test_mul_overflow<int32_t, int32_t, int32_t>(int_min, 0, 0, false);to test_mul_overflow<T, T, T>(min_value, T{0}, T{0}, false);also, please check that they are not already covered by previous tests. |
|
@fbusato hm, if I can still catch the same kind of errors then I think it'd be a good idea. I hardcoded it (even going so far as using std types instead of more generic types whose bitwidth could vary across platforms) because I wanted to be certain that the operand bit sizes were exactly a certain width, even if it came at the expense of readability (they are corner-case tests after all hahah). Let's see what I can whip up! Edit: I'll double-check that there isn't any re-testing going on too. I am only 99% sure since I had kept avoid duplication in mind, but it never hurts to verify :) |
|
@fbusato you know...now that we found that the increase in test run-time was not because of anything inherent in the implementation, let me see whether I could add some of these corner-cases within the section of code that utilize generics. Perhaps I could strike a balance between checking corner-cases without greatly increasing the time it takes to even compile the |
|
@fbusato I remembered why I avoided adding the corner-cases along with the rest of the checks in Is the problem with using explicit types in this PR due to how much harder it becomes to read the code, or is it a preference for generics over explicit std types altogether (or wanting to include everything within test_type)? Perhaps taking an approach like this would be what you were asking for (although the generic type pile up when I have combinations of signed and unsigned bits with 32/64 bit combinations): // 1. Boundary edge-cases
using T1 = cuda::std::int32_t;
using U1 = cuda::std::int32_t;
test_mul_overflow<T1, T1, T1>(int_min, int_min, T1{0}, true);
test_mul_overflow<U1, T1, T1>(2, int_max, U1{int_max} * 2u, false);
test_mul_overflow<U1, U1, U1>(uint_max, 2ull, uint_max - 1ull, true); |
2 participants
Description
While working on optimizing
mul_overflowkernel to resolve #7739, I have been using a local test-suite to catch corner-cases to ensure I don't introduce any regressions. For some odd reason, I am finding that when I added the tests tomul_overflow.pass.cppin the other PR, I'd see an atrocious execution time:This PR (and CI run results) would assure me that the slow times are truly local in scope, while making sure that the current version of
mul_overflowcovers all corner-cases.I also added
test_corner_casesbelowtest_exhaustivesince I didn't want to have it called repeatedly (especially because I'm explicitly controlling types to test the corner cases).Checklist