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

[JSC] Add lock / unlock mechanism to LRU in new wasm BBQ for scratch #10624

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Feb 23, 2023

ffae772

[JSC] Add lock / unlock mechanism to LRU in new wasm BBQ for scratch
https://bugs.webkit.org/show_bug.cgi?id=252869
rdar://105856151

Reviewed by Justin Michaud.

When a register is used for scratch, then we must not evict it while it is used
since (1) we would like to have a register, and (2) there is no way to save and restore scratches.
However we have no mechanism to prevent it, and we crash when running tfjs-wasm in JetStream3.

This patch integrates lock and unlock mechanism for registers allocated for scratches.
This lock / unlock names are derived from DFG's reigster allocator & Baseline JIT's IC register allocators.
We mark scratches as locked, and they are not used for register allocation targets.

We would like to expand this mechanism for the non scratch registers when it is currently used and we would like
to lock it down right now. But in this patch, let's first do it for scratches.

* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::LRU::findMin):
(JSC::Wasm::BBQJIT::LRU::lock):
(JSC::Wasm::BBQJIT::LRU::unlock):
(JSC::Wasm::BBQJIT::ScratchScope::gpr const):
(JSC::Wasm::BBQJIT::ScratchScope::fpr const):
(JSC::Wasm::BBQJIT::ScratchScope::bindGPRToScratch):
(JSC::Wasm::BBQJIT::ScratchScope::bindFPRToScratch):
(JSC::Wasm::BBQJIT::ScratchScope::unbindGPRFromScratch):
(JSC::Wasm::BBQJIT::ScratchScope::unbindFPRFromScratch):

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

670996b

Misc iOS, tvOS & watchOS macOS Linux Windows
🧪 style 🛠 ios ⏳ 🛠 mac ⏳ 🛠 wpe ⏳ 🛠 wincairo
⏳ 🛠 ios-sim ⏳ 🛠 mac-AS-debug 🛠 gtk
🧪 webkitperl ⏳ 🧪 ios-wk2 ⏳ 🧪 api-mac 🧪 gtk-wk2
⏳ 🧪 api-ios ⏳ 🧪 mac-wk1 🧪 api-gtk
⏳ 🛠 🧪 jsc ⏳ 🛠 tv ⏳ 🧪 mac-wk2 ⏳ 🛠 jsc-armv7
⏳ 🛠 🧪 jsc-arm64 ⏳ 🛠 tv-sim ⏳ 🧪 mac-AS-debug-wk2 ⏳ 🧪 jsc-armv7-tests
⏳ 🛠 watch ⏳ 🧪 mac-wk2-stress ⏳ 🛠 jsc-mips
⏳ 🛠 watch-sim ⏳ 🧪 jsc-mips-tests

@Constellation Constellation requested a review from a team as a code owner February 23, 2023 23:46
@Constellation Constellation self-assigned this Feb 23, 2023
@Constellation Constellation added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Feb 23, 2023
@Constellation Constellation force-pushed the eng/JSC-Add-lock--unlock-mechanism-to-LRU-in-new-wasm-BBQ-for-scratch branch 4 times, most recently from 670996b to 60c03f0 Compare February 23, 2023 23:51
Copy link
Contributor

@justinmichaud justinmichaud 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

@Constellation
Copy link
Member Author

Locally test it as built. Landing (since this is new BBQ, not enabled and not tested yet).

@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 24, 2023
https://bugs.webkit.org/show_bug.cgi?id=252869
rdar://105856151

Reviewed by Justin Michaud.

When a register is used for scratch, then we must not evict it while it is used
since (1) we would like to have a register, and (2) there is no way to save and restore scratches.
However we have no mechanism to prevent it, and we crash when running tfjs-wasm in JetStream3.

This patch integrates lock and unlock mechanism for registers allocated for scratches.
This lock / unlock names are derived from DFG's reigster allocator & Baseline JIT's IC register allocators.
We mark scratches as locked, and they are not used for register allocation targets.

We would like to expand this mechanism for the non scratch registers when it is currently used and we would like
to lock it down right now. But in this patch, let's first do it for scratches.

* Source/JavaScriptCore/wasm/WasmBBQJIT.cpp:
(JSC::Wasm::BBQJIT::LRU::findMin):
(JSC::Wasm::BBQJIT::LRU::lock):
(JSC::Wasm::BBQJIT::LRU::unlock):
(JSC::Wasm::BBQJIT::ScratchScope::gpr const):
(JSC::Wasm::BBQJIT::ScratchScope::fpr const):
(JSC::Wasm::BBQJIT::ScratchScope::bindGPRToScratch):
(JSC::Wasm::BBQJIT::ScratchScope::bindFPRToScratch):
(JSC::Wasm::BBQJIT::ScratchScope::unbindGPRFromScratch):
(JSC::Wasm::BBQJIT::ScratchScope::unbindFPRFromScratch):

Canonical link: https://commits.webkit.org/260771@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/JSC-Add-lock--unlock-mechanism-to-LRU-in-new-wasm-BBQ-for-scratch branch from 60c03f0 to ffae772 Compare February 24, 2023 01:40
@webkit-early-warning-system webkit-early-warning-system merged commit ffae772 into WebKit:main Feb 24, 2023
@webkit-commit-queue
Copy link
Collaborator

Committed 260771@main (ffae772): https://commits.webkit.org/260771@main

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

@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants