is_positive_const and is_negative_const broken for (some) casts

[rootjalex]

is_positive_const and is_negative_const don't work for some casts that have defined overflow behavior.

For example:

simplify(int8(uint8(Expr((uint32_t)200543487)))) -> -1

but

is_positive_const(int8(uint8(Expr((uint32_t)200543487)))) -> true
is_negative_const(int8(uint8(Expr((uint32_t)200543487)))) -> false

This PR fixes these bugs, and adds a fuzzing test that validates that the sign of a simplified cast chain is the sign of the original expression.

Leaving as a draft because there are some unfinished pieces:

• Currently, there is not support for checking is_( positive | negative )_const for a cast on a Ramp - is this ever necessary?
• Is it worth making a generic fuzzing header in tests/correctness? I'm also working on a fuzzer for interval arithmetic (that work actually detected the bug in is_positive_const), and many pieces across that fuzzer, this fuzzer, and fuzz_simplify.cpp are close enough that it might be worth merging them.
• In propagating a casted value, what should be done if a float is casted into a non-finite value? (see // TODO: check std::isfinite? in the code added to IROperator.cpp).

[steven-johnson]

Is this ready for review? It's still marked as draft.

[rootjalex]

It is ready for review, I was planning to leave it as a draft until I got answers to the three questions (and applied fixes accordingly).

[steven-johnson]

• Currently, there is not support for checking is_( positive | negative )_const for a cast on a Ramp - is this ever necessary?

I don't know, deferring to @abadams here.

• Is it worth making a generic fuzzing header in tests/correctness? I'm also working on a fuzzer for interval arithmetic (that work actually detected the bug in is_positive_const), and many pieces across that fuzzer, this fuzzer, and fuzz_simplify.cpp are close enough that it might be worth merging them.

Yes, that sounds useful, but as a separate PR (not part of this one)

• In propagating a casted value, what should be done if a float is casted into a non-finite value? (see // TODO: check std::isfinite? in the code added to IROperator.cpp).

I don't know, deferring to @abadams here.

[steven-johnson]

LGTM, but probably want @abadams to address the TODO items before landing.

[abadams]

lossless_cast is quite expensive, and is_positive_const is used in places where it needs to be very cheap. I think we might need to come up with an alternative strategy for handling cast chains. Maybe this condition should just be to.can_represent(c->value.type()) to check for widening casts.

[steven-johnson]

Are these expectations of "expected to be cheap", "known to be expensive" etc documented for each of these?

[abadams]

No, the responsibilities of these things tends to creep over time (e.g. lossless_cast is becoming more and more heavy-duty), so I'm trying to push back. I just know in the past I've called is_positive_const assuming it'll be cheap. The simplifier used to be littered with it, but that was all stripped out. Now it seems to be used for special cases in bounds, is_monotonic, the solver, and some more bounds stuff in vectorization. I think in those contexts the simplifier has already run. If that's the case we should just enshrine that in the comments on is_positive_const (i.e. that it does not do constant folding).

[abadams]

Actually now I'm wondering if this function should handle casts at all. That seems like eager simplification, but in the contexts where is_positive_const is used I think the simplifier has already run. Perhaps this function shouldn't do anything that constant folding would do.

[abadams]

e.g. I wouldn't expect is_positive_const to return true on select(false, 4, 5);

[rootjalex]

That seems reasonable to me, in which case I can remove all cast codes (and the fuzzing test). I just want it to be correct, seeing as it produced incorrect bounds for bounds inference in one of my test cases.

[rootjalex]

Although, one thing to note: not all calls to is_positive_const (and the negative version) are called on simplified expressions, notably the calls in bounds inference.

[abadams]

Hrm. Then having more smarts here does have a positive effect. Is it feasible to insert simplification into the places that might be unsimplified in bounds? Alternatively, maybe we should just have a cheap constant-folding routine that we can call first as an alternative to full simplification. Your propagate functions just handle casts. Yet another alternative would be is_bounded_const, which would take a min and max. This would be nice because it has a natural recursive thing to do when you hit a narrowing cast: reduce the max to the maximum value of the narrower type.

[rootjalex]

I think cheap constant folding as an alternative would be better, as I'm pretty sure no simplification is done in bounds, but constant folding would be very helpful (I often see very complicated expressions produced that simplify down with just constant folding).

I'm not as sure as to the usefulness of is_bounded_const - where would that be used?

[rootjalex]

Well, no simplification other than in can_prove, which actually might be called a lot..

[steven-johnson]

buildbots are green. ready to merge?

[rootjalex]

buildbots are green. ready to merge?

@abadams and I discussed this yesterday - for now, is_positive_const and is_negative_const should only recurse on widening casts. As far as we can tell, the only times that these are being called on non-widening casts of constants are in bounds inference, and we're discussing a different solution to that problem.

I'll push the discussed fix soon, hopefully tonight.

[steven-johnson]

Looks clean and ready to land.