Select condition vector lanes must match the true and false value #8465
Conversation
The ability for the condition to be a scalar while the value is a vector was a mistake that added complexity in a few places. It's easy enough to just check if the condition is a broadcast if you really need to know. One place it added a bunch of complexity is that it meant in the RHS of the simplifier rules, sometimes you needed an implicit broadcast. This was responsible for about a third of the code size in Simplify_Sub.o! It's also unclear if it respects the reduction order we use to prove the simplifier terminates, because those rules were turning an implicit broadcast in the IR into a new actual Broadcast node.
| @@ -1497,7 +1497,7 @@ Expr saturating_cast(Type t, Expr e) { | |||
| Expr select(Expr condition, Expr true_value, Expr false_value) { | |||
| if (as_const_int(condition)) { | |||
| // Why are you doing this? We'll preserve the select node until constant folding for you. | |||
There was a problem hiding this comment.
What's the problem with immediately throwing it away depending on the value of the constant?
There was a problem hiding this comment.
At the time I wrote the original code, I wasn't sure in what circumstances that would ever happen so I was hesistant to not return a Select node. Maybe you're right though. It would probably only be used that way in generic code, where something is sometimes an Expr and sometimes a constant, for confused people who think you can't use a ternary operator in that case to switch between expressions based on a bool, or for people who prefer the select syntax over C++'s or python's ternary operator for some reason. In all of those cases it's fine to eagerly simplify.
|
Failures seem to be vulkan related, for which I believe fixes are in progress. |
In our Select node, I believe the ability for the condition to be a scalar while the value is a vector was a mistake that just adds complexity. It's easy enough to just check if the condition is a broadcast if you really care about that case.
One place it added a bunch of complexity is that it meant in the RHS of the simplifier rules, sometimes you needed an implicit broadcast. These checks for an implicit broadcast on every binary node construction were responsible for about a third of the code size in Simplify_Sub.o! It's also unclear if it respects the reduction order we use to prove the simplifier terminates, because those rules were turning an implicit broadcast in the IR into a new actual Broadcast node, which may increase the total number of IR nodes, which the simplifier is not supposed to do. Now the Broadcast node is there explicitly in the input.
The select helper in IROperator.h can still take scalar conditions - but it now broadcasts them before calling Select::make. This matches the type-matching behavior of the other helpers in IROperator.h.
This shaves ~600k of code size from the compiler, and speeds up compilation of Simplify_Sub.o (and presumably other large simplifier modules) by 20%.