Bug fix for lossless_cast with minor additions #5459
Conversation
The bug can seen for types where lossless_cast type can represent cast->value.type() but not cast->type. For eg: lossless_cast(UInt(16), cast(Int(8), Variable::make(UInt(16), e))) returns (uint16)e which is incorrect. The patch also adds lossless_cast of Mod and Ramp expressions.
|
@dsharletg @abadams PTAL |
src/IROperator.cpp
Outdated
| @@ -566,6 +618,32 @@ Expr lossless_cast(Type t, Expr e) { | |||
| return Shuffle::make(vecs, shuf->indices); | |||
| } | |||
|
|
|||
| if (const Mod *mod = e.as<Mod>()) { | |||
| if (lossless_cast(t, mod->b).defined()) { | |||
There was a problem hiding this comment.
Making some changes for handling negative numbers here
src/IROperator.cpp
Outdated
| @@ -438,6 +439,80 @@ Expr const_false(int w) { | |||
| return make_zero(UInt(1, w)); | |||
| } | |||
|
|
|||
| void check(const Type &t, const Expr &in, const Expr &correct) { | |||
There was a problem hiding this comment.
Rename check_lossless_cast ?
src/IROperator.cpp
Outdated
| << correct << "\n"; | ||
| } | ||
|
|
||
| void lossless_cast_test() { |
There was a problem hiding this comment.
I think our convention would be to put this at the bottom of the file.
|
Ping for review! |
src/IROperator.cpp
Outdated
| @@ -2365,4 +2404,83 @@ Expr undef(Type t) { | |||
| Internal::Call::PureIntrinsic); | |||
| } | |||
|
|
|||
| namespace Internal { | |||
There was a problem hiding this comment.
As a general rule, if it can be an external correctness test, it should be, to keep libHalide smaller. I think this code can be.
src/IROperator.cpp
Outdated
| if (const Mod *mod = e.as<Mod>()) { | ||
| Expr val; | ||
| if (e.type().is_uint()) { | ||
| val = simplify(mod->b + make_const(e.type(), -1)); |
There was a problem hiding this comment.
What happens here and below if mod->b is zero?
src/IROperator.cpp
Outdated
| val = simplify(mod->b + make_const(e.type(), -1)); | ||
| } else if (e.type().is_int()) { | ||
| // 0 <= a%b < |b| | ||
| Type wide_ty = e.type().with_bits(e.type().bits() * 2); |
There was a problem hiding this comment.
In the cases where you return cast(t, e), could you just do this:
Expr x = cast(t, e);
if (can_prove(cast(e.type(), x) == e) {
return x;
}
?
There was a problem hiding this comment.
Or maybe
if (e.type().can_represent(t.min()) &&
e.type().can_represent(t.max()) &&
can_prove(e >= cast(e.type(), t.min()) &&
e <= cast(e.type(), t.max()))
Basically I'm wondering if we can do something that appeals to verified code to reason about the bounds of mods and ramps, rather than having to worry about this method being correct. It's hard to get bounds right.
src/IROperator.cpp
Outdated
| // We can recurse into widening casts. | ||
| // We can recurse into widening casts. | ||
| if (c->type.can_represent(c->value.type()) && | ||
| t.can_represent(c->value.type())) { |
There was a problem hiding this comment.
I think you can get rid of this second part of the condition, and leave that to the recursion?
|
I got rid of Ramp and Mod for lossless_cast as per discussion with Dillon. |
The bug can seen for types where lossless_cast type can represent
cast->value.type() but not cast->type. For eg:
lossless_cast(UInt(16), cast(Int(8), Variable::make(UInt(16), e))) returns
(uint16)e which is incorrect.
The patch also adds lossless_cast of Mod and Ramp expressions.