Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix result height when folding select in WasmBBQJIT #11247

Conversation

ddegazio
Copy link
Contributor

@ddegazio ddegazio commented Mar 8, 2023

c0310d3

Fix result height when folding select in WasmBBQJIT
https://bugs.webkit.org/show_bug.cgi?id=253592
rdar://106420016

Reviewed by Michael Saboff and Yusuke Suzuki.

Currently, when BBQ JIT folds a select instruction, it naively selects
between the Value operands it was given based on the constant condition.
This can sometimes result in a resulting temp with an incorrect stack
height.

This patch changes select to consume all operands, not just the one we
didn't select, similar to the non-folded case. It also sets the result
to the right temp index, if applicable (we can still return a constant,
where the height of the value is irrelevant), and allocates it
independently. Finally, this patch also includes some semi-related
changes to the BBQ JIT move helpers, allowing them to be used to move
values between any two Locations, instead of the current implementation
which requires the source operand to be a Value.

* JSTests/wasm/stress/foldable-select.js: Added.
(async test):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::addSelect):
(JSC::Wasm::BBQJIT::emitStore):
(JSC::Wasm::BBQJIT::emitMoveMemory):
(JSC::Wasm::BBQJIT::emitMoveRegister):
(JSC::Wasm::BBQJIT::emitLoad):
(JSC::Wasm::BBQJIT::emitMove):

Canonical link: https://commits.webkit.org/261461@main

7253ec2

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1   πŸ§ͺ gtk-wk2
βœ… πŸ›  πŸ§ͺ jsc βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2   πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ jsc-arm64 βœ… πŸ›  tv-sim ❌ πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ›  jsc-armv7
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ jsc-armv7-tests
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim βœ… πŸ›  jsc-mips
βœ… πŸ§ͺ jsc-mips-tests

@ddegazio ddegazio requested a review from a team as a code owner March 8, 2023 20:02
@ddegazio ddegazio self-assigned this Mar 8, 2023
@ddegazio ddegazio added the WebAssembly For bugs in JavaScript WebAssembly label Mar 8, 2023
Copy link
Contributor

@msaboff msaboff left a comment

Choose a reason for hiding this comment

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

r=me

@ddegazio ddegazio added the merge-queue Applied to send a pull request to merge-queue label Mar 9, 2023
https://bugs.webkit.org/show_bug.cgi?id=253592
rdar://106420016

Reviewed by Michael Saboff and Yusuke Suzuki.

Currently, when BBQ JIT folds a select instruction, it naively selects
between the Value operands it was given based on the constant condition.
This can sometimes result in a resulting temp with an incorrect stack
height.

This patch changes select to consume all operands, not just the one we
didn't select, similar to the non-folded case. It also sets the result
to the right temp index, if applicable (we can still return a constant,
where the height of the value is irrelevant), and allocates it
independently. Finally, this patch also includes some semi-related
changes to the BBQ JIT move helpers, allowing them to be used to move
values between any two Locations, instead of the current implementation
which requires the source operand to be a Value.

* JSTests/wasm/stress/foldable-select.js: Added.
(async test):
* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::addSelect):
(JSC::Wasm::BBQJIT::emitStore):
(JSC::Wasm::BBQJIT::emitMoveMemory):
(JSC::Wasm::BBQJIT::emitMoveRegister):
(JSC::Wasm::BBQJIT::emitLoad):
(JSC::Wasm::BBQJIT::emitMove):

Canonical link: https://commits.webkit.org/261461@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Fix-result-height-when-folding-select-in-WasmBBQJIT branch from 7253ec2 to c0310d3 Compare March 9, 2023 21:26
@webkit-commit-queue webkit-commit-queue merged commit c0310d3 into WebKit:main Mar 9, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 261461@main (c0310d3): https://commits.webkit.org/261461@main

Reviewed commits have been landed. Closing PR #11247 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebAssembly For bugs in JavaScript WebAssembly
Projects
None yet
5 participants