Skip to content

eliminate multiple return statements for wasm2asm functions#1448

Merged
kripken merged 1 commit into
WebAssembly:masterfrom
froydnj:fix-multiple-returns
Mar 1, 2018
Merged

eliminate multiple return statements for wasm2asm functions#1448
kripken merged 1 commit into
WebAssembly:masterfrom
froydnj:fix-multiple-returns

Conversation

@froydnj

@froydnj froydnj commented Feb 27, 2018

Copy link
Copy Markdown
Contributor

This change eliminates one issue that prevents asm.js validation of the
generated code, see #1443.

kripken
kripken approved these changes Feb 27, 2018
View reviewed changes

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

Not sure what the CI errors are - maybe not all tests were updated?

Comment thread src/wasm2asm.h Outdated
&static_cast<Block*>(func->body)->list : nullptr;
bool endsInReturn =
(isBodyBlock && ((*stats)[stats->size()-1]->_id == Expression::ReturnId));
(isBodyBlock && ((*stats)[stats->size()-1]->_id == Expression::ReturnId)) ||

@kripken kripken Feb 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're here, please change ->_id == Expression::ReturnId to ->is<Return>(). (Also the same happens with Block a few lines above.)

@froydnj

froydnj commented Feb 27, 2018

Copy link
Copy Markdown
Contributor Author

Not sure what the CI errors are - maybe not all tests were updated?

Sigh, apparently CI checks out extra tests in a different repo and runs those, too? How are those supposed to be updated in parallel?

@froydnj

froydnj commented Feb 27, 2018

Copy link
Copy Markdown
Contributor Author

Specifically, address.2asm.js and forward.2asm.js were not updated, because those files don't exist in the binaryen repo, only in the testsuite repo.

@kripken

kripken commented Feb 27, 2018

Copy link
Copy Markdown
Member

Is that the spec repo? I do see foward.wast in there. I guess we already have some code to run the spec test suite through wasm2asm? (which I was thinking we'd want eventually, surprised it's already started)

Perhaps the only thing missing is to add a few lines to ./auto_update_tests.py for updating those, as the inputs are in the spec test repo, but the outputs are in this one.

@froydnj

froydnj commented Feb 27, 2018

Copy link
Copy Markdown
Contributor Author

Oh, I see, I don't have to update a separate repo, I just have to have the repo checked out, and auto_update_tests.py will take care of those automatically. I was confused!

@froydnj froydnj force-pushed the fix-multiple-returns branch from 594ca6b to e4d0710 Compare February 27, 2018 21:56
@kripken

kripken commented Feb 27, 2018

Copy link
Copy Markdown
Member

Last errors look like binaryen.js, which means we need to update the wasm2asm component in the JS port too. If you have emscripten set up ./build-js.sh can do that. If not I can do it for you.

@froydnj froydnj force-pushed the fix-multiple-returns branch 2 times, most recently from 200f05b to 04332cf Compare February 28, 2018 15:39
@froydnj

froydnj commented Feb 28, 2018

Copy link
Copy Markdown
Contributor Author

@kripken I don't know where to go from here. The test passes locally for me, and I think I've regenerated the JS files correctly. Can you try regenerating the files?

@kripken

kripken commented Feb 28, 2018

Copy link
Copy Markdown
Member

I think the only problem left here might be the merge conflict. I merged in master and rebuilt here and it looks fine. We can just merge that branch in, or you can merge it into here, either way.

@froydnj froydnj force-pushed the fix-multiple-returns branch from 04332cf to 7d8462d Compare February 28, 2018 22:41
@froydnj

froydnj commented Mar 1, 2018

Copy link
Copy Markdown
Contributor Author

Sigh, last thing to fix:

executing:  bin/asm2wasm /home/travis/build/WebAssembly/binaryen/test/two_sides.asm.js --trap-mode=clamp -O0
[PassRunner] running passes...
[PassRunner]   running pass: finalize-calls...        0.0004724 seconds.
[PassRunner]   (validating)
The job exceeded the maximum time limit for jobs, and has been terminated.

Is this a known potential problem? I guess it's possible that the change might have eliminated a return statement that was there before, and thus the test never returns, or something?

@kripken

kripken commented Mar 1, 2018

Copy link
Copy Markdown
Member

I've seen that before, it seemed like a random CI failure. Might be ok after a restart.

@froydnj
eliminate multiple return statements for wasm2asm functions
9affe82 
This change eliminates one issue that prevents asm.js validation of the
generated code, see WebAssembly#1443.
@froydnj froydnj force-pushed the fix-multiple-returns branch from 7d8462d to 9affe82 Compare March 1, 2018 14:29
@froydnj

froydnj commented Mar 1, 2018

Copy link
Copy Markdown
Contributor Author

I've seen that before, it seemed like a random CI failure. Might be ok after a restart.

Hm, OK. Had to fix another merge conflict anyway, so rebased and pushed again. We'll see if CI is happy now.

@kripken

kripken commented Mar 1, 2018

Copy link
Copy Markdown
Member

Looks good! :)

@kripken kripken merged commit 363c2c7 into WebAssembly:master Mar 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kripken kripken kripken approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@froydnj @kripken