Skip to content

Move wasm refs back to Int64s on 32-bits. #33900

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

Conversation

aoikonomopoulos
Copy link
Contributor

@aoikonomopoulos aoikonomopoulos commented Sep 19, 2024

44c025e

Move wasm refs back to Int64s on 32-bits.
https://bugs.webkit.org/show_bug.cgi?id=279977

Reviewed by Justin Michaud and Keith Miller.

Lots of generic code carries the assumption that refs are stored as
EncodedJSValues. In the interest of getting things working, change
wasm.json to use Int64 for refs (rather than pointerType()).

Update ref-accessing code in WasmOMGIRGenerator32_64.cpp to match.

* Source/JavaScriptCore/wasm/WasmOMGIRGenerator32_64.cpp:
(JSC::Wasm::OMGIRGenerator::addRefIsNull):
(JSC::Wasm::OMGIRGenerator::addTableGet):
(JSC::Wasm::OMGIRGenerator::addRefAsNonNull):
(JSC::Wasm::OMGIRGenerator::addI31GetS):
(JSC::Wasm::OMGIRGenerator::addI31GetU):
(JSC::Wasm::OMGIRGenerator::pushArrayNew):
(JSC::Wasm::OMGIRGenerator::addArrayGet):
(JSC::Wasm::OMGIRGenerator::emitArrayNullCheck):
(JSC::Wasm::OMGIRGenerator::addArrayLen):
(JSC::Wasm::OMGIRGenerator::addStructNew):
(JSC::Wasm::OMGIRGenerator::addStructNewDefault):
(JSC::Wasm::OMGIRGenerator::addStructGet):
(JSC::Wasm::OMGIRGenerator::addStructSet):
(JSC::Wasm::OMGIRGenerator::emitRefTestOrCast):
(JSC::Wasm::OMGIRGenerator::addBranchNull):
(JSC::Wasm::OMGIRGenerator::addCallRef):
* Source/JavaScriptCore/wasm/wasm.json:

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

6e47f4e

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 win
✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 win-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv ❌ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@aoikonomopoulos aoikonomopoulos requested a review from a team as a code owner September 19, 2024 13:33
@aoikonomopoulos aoikonomopoulos self-assigned this Sep 19, 2024
@aoikonomopoulos aoikonomopoulos added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Sep 19, 2024
Copy link
Contributor

@kmiller68 kmiller68 left a comment

Choose a reason for hiding this comment

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

Can we make a using WasmConstRefValue = Const64Value and use that? That will help enumerate places to update if you ever want to switch back.

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 once Keith is happy

@aoikonomopoulos aoikonomopoulos force-pushed the eng/Move-wasm-refs-back-to-Int64s-on-32-bits- branch from 26042d8 to 2b27886 Compare September 20, 2024 10:35
@aoikonomopoulos aoikonomopoulos force-pushed the eng/Move-wasm-refs-back-to-Int64s-on-32-bits- branch from 2b27886 to 6e47f4e Compare September 23, 2024 08:37
@aoikonomopoulos
Copy link
Contributor Author

@kmiller68 made the changes in the new upload, thanks.

Copy link
Contributor

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

@aoikonomopoulos aoikonomopoulos added the merge-queue Applied to send a pull request to merge-queue label Sep 23, 2024
https://bugs.webkit.org/show_bug.cgi?id=279977

Reviewed by Justin Michaud and Keith Miller.

Lots of generic code carries the assumption that refs are stored as
EncodedJSValues. In the interest of getting things working, change
wasm.json to use Int64 for refs (rather than pointerType()).

Update ref-accessing code in WasmOMGIRGenerator32_64.cpp to match.

* Source/JavaScriptCore/wasm/WasmOMGIRGenerator32_64.cpp:
(JSC::Wasm::OMGIRGenerator::addRefIsNull):
(JSC::Wasm::OMGIRGenerator::addTableGet):
(JSC::Wasm::OMGIRGenerator::addRefAsNonNull):
(JSC::Wasm::OMGIRGenerator::addI31GetS):
(JSC::Wasm::OMGIRGenerator::addI31GetU):
(JSC::Wasm::OMGIRGenerator::pushArrayNew):
(JSC::Wasm::OMGIRGenerator::addArrayGet):
(JSC::Wasm::OMGIRGenerator::emitArrayNullCheck):
(JSC::Wasm::OMGIRGenerator::addArrayLen):
(JSC::Wasm::OMGIRGenerator::addStructNew):
(JSC::Wasm::OMGIRGenerator::addStructNewDefault):
(JSC::Wasm::OMGIRGenerator::addStructGet):
(JSC::Wasm::OMGIRGenerator::addStructSet):
(JSC::Wasm::OMGIRGenerator::emitRefTestOrCast):
(JSC::Wasm::OMGIRGenerator::addBranchNull):
(JSC::Wasm::OMGIRGenerator::addCallRef):
* Source/JavaScriptCore/wasm/wasm.json:

Canonical link: https://commits.webkit.org/284070@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Move-wasm-refs-back-to-Int64s-on-32-bits- branch from 6e47f4e to 44c025e Compare September 23, 2024 13:22
@webkit-commit-queue
Copy link
Collaborator

Committed 284070@main (44c025e): https://commits.webkit.org/284070@main

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

@webkit-commit-queue webkit-commit-queue merged commit 44c025e into WebKit:main Sep 23, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Sep 23, 2024
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.

5 participants