wasm-as: select operator arguments emitted in reverse order.

Example:

(module
  (memory 0 0)
  (func $f1 (param $i1 i32) (result i32)
    (i32.select
     (i32.lt_s
      (get_local $i1)
      (i32.const 0)
      )
     (i32.sub
      (i32.const 0)
      (get_local $i1)
      )
     (get_local $i1)
     )
    (i32.const 0))
  )

sexp-wasm emits the select operator arguments in the wast order.

0000010: 05                                         ; OPCODE_SELECT
0000011: 4f                                         ; OPCODE_I32_LT_S
0000012: 0e                                         ; OPCODE_GET_LOCAL
0000013: 00                                         ; remapped local index
0000014: 09                                         ; OPCODE_I8_CONST
0000015: 00                                         ; u8 literal
0000016: 41                                         ; OPCODE_I32_SUB
0000017: 09                                         ; OPCODE_I8_CONST
0000018: 00                                         ; u8 literal
0000019: 0e                                         ; OPCODE_GET_LOCAL
000001a: 00                                         ; remapped local index
000001b: 0e                                         ; OPCODE_GET_LOCAL
000001c: 00                                         ; remapped local index
000001d: 09                                         ; OPCODE_I8_CONST
000001e: 00                                         ; u8 literal

Interestingly wasm-as emits them in the opposite order, that is the i32.sub first then the i32.lt_s.

[kripken]

The v8 document specifies the condition is at the end, so that's what binaryen is doing.

[sunfishcode]

WebAssembly/design#489 is a PR to update the design repo to put the condition at the end.

@kripken Checking the actual output of v8. The sexp-wasm has a test/d8/select.txt that includes:

(func $i32 (param i32) (result i32)
    (i32.select (get_local 0) (i32.const 1) (i32.const 2)))
... 
   (call $i32 (i32.const 0))

which generates

test_i32_l() = 2

and is encoded as

; function 0
0000016: 00                                         ; func flags
0000017: 0000                                       ; func signature index
0000019: 0000                                       ; func body size
000001b: 05                                         ; OPCODE_SELECT
000001c: 0e                                         ; OPCODE_GET_LOCAL
000001d: 00                                         ; remapped local index
000001e: 09                                         ; OPCODE_I8_CONST
000001f: 01                                         ; u8 literal
0000020: 09                                         ; OPCODE_I8_CONST
0000021: 02                                         ; u8 literal

So it seems to be a documentation error.

[kripken]

Yeah, those two don't match, but (1) binaryen should usually follow the binary spec, not an implementation of it, and (2) as @sunfishcode, there is reason to believe that what the binary spec says is what should be.

It seems fair to see the document as derived from the implement in this case. The issue that @sunfishcode refers to is a change request for select not a clarification of the current state (from my reading of it anyway). Not sure why there has been no action of the select change, but conflating it with a br_if change might not have helped.

@titzer Need some clarification on this one too?

[titzer]

The condition should go at the end. I had a CL that did that in the native
prototype but didn't land it pending the other PRs. I'll make sure V8 is
up-to-date and ping this thread when it is.

On Fri, Jan 22, 2016 at 2:51 AM, JSStats notifications@github.com wrote:

It seems fair to see the document as derived from the implement in this
case. The issue that @sunfishcode https://github.com/sunfishcode refers
to is a change request for select not a clarification of the current
state (from my reading of it anyway). Not sure why there has been no action
of the select change, but conflating it with a br_if change might not
have helped.

@titzer https://github.com/titzer Need some clarification on this one
too?

—
Reply to this email directly or view it on GitHub
#131 (comment)
.

[kripken]

Ok, cool, then nothing to do in binaryen, I think we can close this.

@titzer If you support change select to put the condition last, which I am not arguing against, then could you please consider approving @sunfishcode PR in WebAssembly/design#489 and just the select change if that is all there is consensus on. I would just like the tools to generate code compatible with the spec and implementations for a start - to get something running.

[titzer]

CL in flight: https://codereview.chromium.org/1624323003/

On Sat, Jan 23, 2016 at 10:37 PM, JSStats notifications@github.com wrote:

@titzer https://github.com/titzer If you support change select to put
the condition last, which I am not arguing against, then could you please
consider approving @sunfishcode https://github.com/sunfishcode PR in
WebAssembly/design#489 WebAssembly/design#489
and just the select change if that is all there is consensus on. I would
just like the tools to generate code compatible with the spec and
implementations for a start - to get something running.

—
Reply to this email directly or view it on GitHub
#131 (comment)
.