Repository navigation
Incorrect optimizer result with if parameter #5950
Description
The following program:
(module
(func $f (export "f") (param $x i32) (param $y i32) (result i32)
local.get $x
local.get $y
if (param i32)
return
else
drop
local.get $y
return
end
unreachable
)
)is optimized to:
(module
(type $0 (func (param i32 i32) (result i32)))
(export "f" (func $0))
(func $0 (param $0 i32) (param $1 i32) (result i32)
(drop
(return
(local.get $0)
)
)
(if
(local.get $1)
(block
)
(block
)
)
(unreachable)
)
)by
wat2wasm weird.wat && wasm-merge -S weird.wasm bug --output=-
and the function body becomes simply (local.get $0) with wasmopt -O.
I am surprised by the fact that the return and return_call are lifted out of the if block. This does not happen when the local.get $x is moved inside both if alternatives and the if does not take a parameter.
With the original module, the following tests pass, but they fail with the binaryen output, so I think this is a bug:
(assert_return (invoke "f" (i32.const 100) (i32.const 1)) (i32.const 100))
(assert_return (invoke "f" (i32.const 100) (i32.const 0)) (i32.const 0))This is on the current main (16a5993).
Activity
[-]Unexpected optimizer result with if parameter[/-][+]Incorrect optimizer result with if parameter[/+]kripken commentedon Sep 18, 2023
Looks like a binary parsing error, which I think is because of Binaryen's lack of support for multivalue input params to control flow structures.
We should probably error on any such control flow structures?
camilstaps commentedon Sep 19, 2023
@kripken Thanks. An error would have been helpful, yes.
When I add the
--enable-multivalueflag to thewasm-mergecall the result is the same, though. Is multi-value not fully implemented?wasm-merge --helpsays it enables "multivalue functions", should I interpret that as support for multi-valuedreturnbut not control flow structure parameters? If so, it may be useful to clarify that as well. I had read the readme and #663, but when I saw--enable-multivaluejust assumed they were outdated.Are there any plans for supporting control flow structure parameters? (I originally wanted to use LLVM's
wasm-ldinstead ofwasm-mergefor reasons like this, but came back towasm-mergebecausewasm-lddoesn't fully support tables.)kripken commentedon Sep 19, 2023
Yes, I think "multivalue functions" is meant to indicate that the support is only present for function returns and calls of such functions (and the result of inlining such things). I agree that might not be clear enough. Really we should have an error in the binary parser for this. I'll try to look into that.
I think our plan is to support parsing of control flow inputs with the new wasm parser @tlively is working on? That should fix correctness at least, though optimization would take more work. Note that in general this is not a top priority for us as the benefits of such code are minor, 1-2% at best, so there are better things to focus on.
tlively commentedon Sep 19, 2023
I wasn't planning on it before, but yes, the new wat parser and associated IRBuilder should make it much more feasible to support parsing multivalue control structures from binary and text. We'll be replacing the input parameters with locals, though, unless we decide to make larger changes to Binaryen IR.
Error on multivalue inputs that we do not handle (#5962)
camilstaps commentedon Sep 21, 2023
Thanks both, and thanks for the new error! I’ll leave it up to you if you want to leave this issue open.
tlively commentedon Apr 15, 2024
Closing this in favor of #6407.
Error on multivalue inputs that we do not handle (WebAssembly#5962)
tlively commentedon Jan 11, 2025
FWIW control flow parameters are now supported.