Require `then` and `else` with `if` by tlively · Pull Request #6201 · WebAssembly/binaryen

tlively · 2024-01-04T19:52:03Z

We previously supported (and primarily used) a non-standard text format for
conditionals in which the condition, if-true expression, and if-false expression
were all simply s-expression children of the if expression. The standard text
format, however, requires the use of then and else forms to introduce the
if-true and if-false arms of the conditional. Update the legacy text parser to
require the standard format and update all tests to match. Update the printer to
print the standard format as well.

The .wast and .wat test inputs were mechanically updated with this script:
https://gist.github.com/tlively/85ae7f01f92f772241ec994c840ccbb1

We previously supported (and primarily used) a non-standard text format for conditionals in which the condition, if-true expression, and if-false expression were all simply s-expression children of the `if` expression. The standard text format, however, requires the use of `then` and `else` forms to introduce the if-true and if-false arms of the conditional. Update the legacy text parser to require the standard format and update all tests to match. Update the printer to print the standard format as well. The .wast and .wat test inputs were mechanically updated with this script: https://gist.github.com/tlively/85ae7f01f92f772241ec994c840ccbb1

tlively · 2024-01-04T19:52:15Z

Current dependencies on/for this PR:

main
- PR Require then and else with if #6201 👈
  - PR [Parser] Parse br_if correctly #6202

This stack of pull requests is managed by Graphite.

tlively · 2024-01-04T19:54:47Z

I strongly recommend hiding whitespace when viewing the diff 😆

kripken

Ah, this must have changed when wasm switched to a stack machine... because it is needed to identify structural children.

lgtm % questions

kripken · 2024-01-04T20:17:50Z

test/gtest/cfg.cpp

 4:
  6: i32.const 3
-  7: block
+  7: block (result i32)


Unrelated change?

No, this is a necessary change. Probably due to a difference in finalization between the parsing for block and the parsing for then, but not sure it's worth digging into more.

Makes sense, it's probably that.

kripken · 2024-01-04T20:34:25Z

test/wasm2js/fac.2asm.js

-   $12$hi = i64toi32_i32$2;
+  if (($0_1 | 0) == (i64toi32_i32$3 | 0) & ($0$hi | 0) == (i64toi32_i32$1 | 0) | 0) {
+   $8 = 1;
+   $8$hi = 0;


I am surprised to see codegen changes here. This change even looks non-NFC. Do you know why this changed?

It looks like the source file contains then and else blocks containing single instructions. Those were previously parsed as singleton blocks, but now they're parsed as the actual instructions. I think that change to the IR probably explains the different output here.

Could be. But looking at the output it's not obvious to me if it is or isn't a regression (line 21 is gone, for example - these aren't just renamed variables, there is a structural change), so I think it's worth investigating. Let me know if you'd like help with that.

Ok, I verified that the different parsing behavior explains this. I ran wasm2js on one of the source functions with and without the special case for singleton then and else blocks and the output had precisely these differences.

Great, thanks!

tlively · 2024-01-04T22:25:02Z

Merge activity

Jan 4, 5:25 PM: @tlively started a stack merge that includes this pull request via Graphite.
Jan 4, 5:25 PM: @tlively merged this pull request with Graphite.

kripken · 2024-01-04T23:39:19Z

I see a fuzzer error shortly after this landed which bisects down to this:

running testcase handler: Wasm2JS                                                              
wasm-opt out/test/a.wasm --legalize-js-interface -o /usr/local/google/home/azakai/Dev/2-binaryen
/out/test/a.wasm.temp.wasm                                                                                                                                                                    
wasm-opt out/test/a.wasm.temp.wasm -o out/test/a.wa
sm.temp.wasm --stub-unsupported-js --dealign --alignment-lowering
wasm-opt out/test/a.wasm.temp.wasm -o out/test/b.wa
sm.temp.wasm --roundtrip -O1 --optimize-casts --code-pushing --simplify-locals-notee
wasm-opt out/test/a.wasm.temp.wasm --emit-js-wrapper=/dev/stdout
warning: no passes specified, not doing any work
warning: no output file specified, not emitting output
wasm2js out/test/a.wasm.temp.wasm --emscripten -O --deterministic
terminate called after throwing an instance of 'wasm::SParseException'

That is on seed 10441444793052190197.

Reduced testcase:

(module
 (type $0 (func (result i32)))
 (func $0 (result i32)
  (i32.popcnt
   (i32.const 0)
  )
 )
)

STR:

$ bin/wasm-as w.wat -o a.wasm
$ bin/wasm2js a.wasm
terminate called after throwing an instance of 'wasm::SParseException'
Aborted

Oddly it's not a text format issue, as it happens when parsing a binary..?

@tlively

The intrinsics file changed in #6201 and somehow CMake doesn't automatically update itself, and needs a manual step for people with existing checkouts (a new fresh checkout always works). To avoid annoyance for existing checkouts, rename the vars, which forces CMake to recompute the contents.

We previously supported (and primarily used) a non-standard text format for conditionals in which the condition, if-true expression, and if-false expression were all simply s-expression children of the `if` expression. The standard text format, however, requires the use of `then` and `else` forms to introduce the if-true and if-false arms of the conditional. Update the legacy text parser to require the standard format and update all tests to match. Update the printer to print the standard format as well. The .wast and .wat test inputs were mechanically updated with this script: https://gist.github.com/tlively/85ae7f01f92f772241ec994c840ccbb1

The intrinsics file changed in WebAssembly#6201 and somehow CMake doesn't automatically update itself, and needs a manual step for people with existing checkouts (a new fresh checkout always works). To avoid annoyance for existing checkouts, rename the vars, which forces CMake to recompute the contents.

tlively requested a review from kripken January 4, 2024 19:52

changelog

e54a918

kripken reviewed Jan 4, 2024

View reviewed changes

tlively mentioned this pull request Jan 4, 2024

[Parser] Parse br_if correctly #6202

Merged

kripken approved these changes Jan 4, 2024

View reviewed changes

tlively merged commit a58281c into main Jan 4, 2024

tlively deleted the standard-if-then-else branch January 4, 2024 22:25

kripken mentioned this pull request Jan 5, 2024

Rename CMake vars for modified intrinsics file #6204

Merged

gkdn mentioned this pull request Aug 31, 2024

stringconsts gkdn/binaryen#1

Closed

Conversation

tlively commented Jan 4, 2024

Uh oh!

tlively commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlively commented Jan 4, 2024

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlively commented Jan 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

kripken commented Jan 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tlively commented Jan 4, 2024 •

edited

Loading

tlively commented Jan 4, 2024 •

edited

Loading