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
I don't know how important it is, but changing |
|
Sure we could backport that. It was just the tip of the iceberg when it came to bugs in lossless_cast though, and fixing the other ones is what required all the work. |
|
Tests are breaking because vulkan is busted on main, and because the fuzzer and asan builds are still busted on linux-worker-1. This is ready for review. |
@derek-gerstmann is working on the Vulkan bug, but one of us should figure out the fuzzer/asan issue |
| @@ -235,6 +231,25 @@ Expr random_expr(std::mt19937 &rng) { | |||
| } | |||
| } | |||
|
|
|||
| bool definitely_has_ub(Expr e) { | |||
There was a problem hiding this comment.
what, no comment about nasal demons?
There was a problem hiding this comment.
LGTM but @vksnk should probably review too and he's out most of this week.
I'll do a full test of this inside Google later today.
| if (target.has_feature(Target::AVX512) && op->type.lanes() >= 32) { | ||
| ConstantInterval ca = constant_integer_bounds(a); | ||
| ConstantInterval cb = constant_integer_bounds(b); | ||
| if (!ca.contains(-32768) || !cb.contains(-32768)) { |
There was a problem hiding this comment.
-0x8000 is probably clearer?
There was a problem hiding this comment.
Anyone reading this code who doesn't know the bit layout of -32768 should not be reading this code :-)
…/halide/Halide into abadams/fix_lossless_cast_of_sub
|
Is this now good to merge? I think it was contingent on the results of tests inside Google. |
|
Not yet -- need to retest in Google with the Xtensa codegen fix in place, but I'm OOO for at least another day or two (out of town due to family medical emergency. Can it wait till then? |
|
Yes, there's no rush, I just wanted to make sure it's not stalled on me. |
|
Xtensa codegen is fixed now, so I re-run tests in Google codebase: there are a couple of tests which now produce mismatch with thegoldens (in one of the tests, the max diff is 6 out of 255, which seems fairly significant), but I guess this is somewhat expected? |
|
Are the tests that differ using floats in some way that would be unstable? (e.g. using a floating-point comparison to decide on an interpolation direction in a demosaicking algorithm) If they're fixed-point algorithms that's definitely not expected. |
|
It's hard to tell for sure, because it's one of those large end-to-end tests which can call 10 different Halide functions deep inside, but I'd guess it does use some floating-point math (I'm not 100%, but I think one of these tests was failing before due to some FP differences before, so we needed to regenerate the goldens; although, the diff was smaller I think). |
|
Do you think it's worth testing if it passes on main under strict_float, and fails on this branch under strict_float? If that's the case there's probably something to worry about. |
|
Sure, I'll see if it can be narrowed down. |
|
Update on Google: it looks like there are no real red flags here -- just goldens to be regenerated -- so I'm hoping to be able to approve this for landing later today pending confirmation. |
|
Approved and landed in Google, so I'm landing this here now. |
This is one thing breaking the SVE2 pr.