Skip to content

Fix stack pointer identification for wasm::ABI::getStackSpace().#2243

Merged
kripken merged 6 commits intoWebAssembly:masterfrom
wmaddox:master
Jul 28, 2019
Merged

Fix stack pointer identification for wasm::ABI::getStackSpace().#2243
kripken merged 6 commits intoWebAssembly:masterfrom
wmaddox:master

Conversation

@wmaddox
Copy link
Copy Markdown
Contributor

@wmaddox wmaddox commented Jul 20, 2019

Recent stack pointer simplification in Emscripten broke the --spill-pointers
pass. This fix for #2229 restores this functionality by recognizing an
alternative coding idiom in Emscripten-generated WASM code.

William Maddox added 2 commits July 19, 2019 17:06
Recent stack pointer simplification in Emscripten broke the --spill-pointers
pass.  This fix for WebAssembly#2229 restores this functionality by recognizing an
alternative coding idiom in Emscripten-generated WASM code.
@wmaddox wmaddox marked this pull request as ready for review July 20, 2019 07:16
Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @wmaddox!

There is still the issue of the stack direction - the current code assumes the stack goes up, but in the wasm backend it goes down. I'm not sure how best to handle that - one option is to make SpillPointers aware of it (a variant with a downward stack), another is to add a PassOption, as this might be relevant for other places too eventually.

Comment thread src/abi/stack.h
Comment thread src/abi/stack.h Outdated
Comment thread src/abi/stack.h Outdated
Copy link
Copy Markdown
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Thanks @wmaddox!

Looks good, two final comments below, and also please add a test for this: there is test/passes/spill-pointers.wast already, can add another module right after it in the same file, then after running ./auto_update_tests.py it will update the expected output with that.

Comment thread src/abi/stack.h Outdated
Comment thread src/abi/stack.h Outdated
@kripken
Copy link
Copy Markdown
Member

kripken commented Jul 28, 2019

Perfect, thanks @wmaddox!

@kripken kripken merged commit be6f7c4 into WebAssembly:master Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants