Incorrect optimizer result with if parameter

[camilstaps]

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 (16a59938563c93d8459bf36679c83497aeba7cc7).

[kripken]

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]

@kripken Thanks. An error would have been helpful, yes.

When I add the --enable-multivalue flag to the wasm-merge call the result is the same, though. Is multi-value not fully implemented? wasm-merge --help says it enables "multivalue functions", should I interpret that as support for multi-valued return but 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-multivalue just assumed they were outdated.

Are there any plans for supporting control flow structure parameters? (I originally wanted to use LLVM's wasm-ld instead of wasm-merge for reasons like this, but came back to wasm-merge because wasm-ld doesn't fully support tables.)

[kripken]

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]

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.

[camilstaps]

Thanks both, and thanks for the new error! I’ll leave it up to you if you want to leave this issue open.

[tlively]

Closing this in favor of #6407.