Should operator<< and operator>> type promotion issue warnings/errors?

[steven-johnson]

In C++, an expression like x << y will always produce a result of type x, regardless of the type of y.

In Halide, though, we use match_bits, so the resulting type is really the type of x but maybe wider. This can be surprising and (especially for simd code) much less efficient than expected (eg some_u8 << some_i32 will take 4x the vector width that you expect).

Having the LHS and RHS match width is desirable for efficient simd use, of course, but it's an easy mistake to make in Halide (ask me how I know). Should we consider tightening the rules so that the RHS must exactly equal the width of the LHS (issue error or warning otherwise)?

[abadams]

I think some_u8 << 8 doesn't widen, right? If that's widening we have a straight-out bug somewhere. So the issue would be code like:

Expr foo = 8;
some_u8 << foo;

I never know what the types should be for shift operators. Why must they match in the first place? I think @dsharletg has thought about this recently.

[dsharletg]

I agree shift types shouldn't need to match.

It looks like this was last changed in 181c579.

It seems OK to me to change the call from match_bits to just casting the RHS to have the same number of bits as the LHS (but it must preserve signedness of the RHS).

[zvookin]

We have been up and down on this N times and the current behavior is there because there were actual bugs and misfeatures with other attempts. I don’t know if any of this changed recently, but the bar for designing the correct behavior is higher than this and if it is a performance issue, well that’s better than a correctness one.

…

On Thu, Jun 25, 2020 at 4:49 PM Dillon Sharlet ***@***.***> wrote: I agree shift types shouldn't need to match. It looks like this was last changed in 181c579 <181c579> . It seems OK to me to change the call from match_bits to just casting the RHS to have the same number of bits as the LHS (but it must preserve signedness of the RHS). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <https://github.com/halide/Halide/issues/5072#issuecomment-649872269>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALOUFD2TBEDSPM2RFFLQH3RYPPARANCNFSM4OIYAQXA> .

[zvookin]

Also "In C++, an expression like x << y will always produce a result of type x, regardless of the type of y." is not correct. x undergoes standard integral promotions, so it maintains its signed/unsignedness and becomes either int or long. Generally the C approach is that int is the one size fits all type for anything you need to do unless you're weird, and if you're weird, you should know how to use casts. (We won't comment on automatic promotions in the existence of signed/unsigned.) Halide's current design tries to keep the number of lanes constant by taking the max of the two widths. There are legit cases where the width ends up unexpectedly too small if the RHS is ignored and appealing to C's design is wrong because C always promotes to an int or uint.

[dsharletg]

Zalman makes good points. Also, if the RHS is wider than the LHS, then casting the RHS really is only affecting the shift itself, and not all of the (likely) inefficient computations leading up to it.

I still think it'd be reasonable to change the behavior here, but I also now remember a lot of challenges in existing code that relied on the current behavior, and the benefit doesn't actually seem that big. Steven, maybe some context on the issue you are seeing would help.