Open bdwjn opened 2 months ago
This could be improved, yeah, looks like a current limitation.
I think what that linked code is referring to is the ordering issue when the first arm has effects, e.g.
(select
(call $before)
(call $later)
(i32.const 1)
)
Here we want to return the call to before, since the condition is true. But later must be called after. So we need to do something like this:
(local.set $temp (call $before))
(drop (call $later))
(local.get $temp)
That may be faster (if VMs do not optimize it, which I am not certain of), but it is around the same size in the binary format.
I still don't get why you need that local in your example. When you flatten the select
it becomes:
(call $before) ;; stack is [T]
(call $later) ;; stack is [F] -> T
;; At this point we have 2 values (of the same type) on the stack, validation guarantees this.
;; We also ran both sides in the correct order, so any side effects have happened the way
;; they should. All that's left to do is get the correct value and drop the other one:
(i32.const 1) ;; stack is [1] -> F -> T
(select) ;; stack is [T]
So that (i32.const 1) (select)
can be replaced with a (drop)
to get the True value, and it won't change the side effects on the code above it.
Yes, in the wasm binary format you can do "stacky" things like that, but not in Binaryen IR, which is more structured. But, good point, since in the time after that comment was written we have added stacky optimizations (StackIR), which can do this. So perhaps it is worth doing now.
Consider the following expression:
Binaryen will try to eliminate the
select
statement when its condition is constant.The
div_u
might cause a trap though, so under safe optimizations (without-tnh
or-iit
) it needs to be preserved.Binaryen outputs both arms, and drops the value we don't need:
Now let's look at the opposite case:
Surely it will use the same trick, right? Output both arms, drop the one you don't need:
Except it doesn't! For some reason it completely gives up here, and keeps the
select
and its condition as-is.The code mentions something about needing to "reverse the order using a temp local" but that makes no sense to me.