Fix horrifying bug in lossless_cast of a subtract #8155
Conversation
|
Please sir, may I have a test? |
|
Hahahaha, I'm giggling for minutes now... 😆 |
|
Is this a recent injection? If not, might want to backport to 17... |
steven-johnson
added
the
backport me
No, 4 years ago: |
|
I added a fuzz test, and there are more bugs :( I'm pretty ready to state that all new features or algorithms must come with a fuzz tester. |
|
This is why people measure test coverage. It's kinda annoying, but probably a thing to consider. |
TODO: - Dedup the constant integer code with the same code in the simplifier. - Move constant interval arithmetic operations out of the class. - Make the ConstantInterval part of the return type of lossless_cast (and turn it into an inner helper) so that it isn't constantly recomputed.
|
where do we stand on this -- do we need to do more work (tests, etc) before landing? |
|
Still working on it. I couldn't figure out how to fix the remaining bugs using top-down reasoning about the types like what was there already, so I rewrote it to use bottom-up constant-integer bounds analysis, using code lifted out of the simplifier. It's working but there are inefficiencies to fix and some code deduplication to do. |
Also added more TODOs
In particular, we can do better instruction selection for pmulhrsw
Also fix up Monotonic.cpp
I can't think of a single case that could cause this
|
lossless_negate has a similar bug. The mul visitor just tries to negate each arg, but if you have e.g. an int8 type, and the product is exactly -128 (e.g. -64 * 2), then a lossless negate just returns -128 again, which hasn't negated anything. |
Also explicit ignore lossless_cast bugs that will be fixed in #8155
* Make ConstantInterval more of a first-class thing and use it in Monotonic.cpp * Restore bound_correlated_differences calls * Elaborate on TODO * Handle some TODOs Also explicit ignore lossless_cast bugs that will be fixed in #8155 * Fix constant interval mod, clean up constant interval saturating cast * Improve comment * Avoid unsigned overflow * Fix the most obvious bug in lossless_cast, to make the fuzzer pass more * Skip over pipelines that fail the lossless_cast check * Drop iteration count on lossless_cast test * Add test to CMakeLists.txt * Avoid UB in constant_interval test (signed integer overflow of the scalars) * Restore accidentally-deleted line from CMakeLists.txt * Print on success * Handle Lets in constant_integer_bounds Also, plumb the cache through the recursive calls * Delete duplicate operator<< * Just always cast the bounds back to the range of the op type * Address review comments * Redo operator<< for ConstantIntervals * Improve comment; disable buggy code for now
|
Status Update? I know this had a lot of other stuff to fix first, just not sure where this stands. |
|
The next PR is going to be reworking the simplifier to use the recently-merged ConstantInterval (pulling that out of this PR), and then after that's in this PR will be updated and hopefully made much smaller. |
This is one thing breaking the SVE2 pr.