[JSC] Move aINT / mINT / uINT bytecode under RTT#64302
Conversation
|
EWS run on previous version of this PR (hash e7847d6) Details
|
e7847d6 to
90b9efe
Compare
|
EWS run on previous version of this PR (hash 90b9efe) Details
|
90b9efe to
4566606
Compare
|
EWS run on previous version of this PR (hash 4566606) Details
|
macOS Safer C++ Build #100576 (4566606)❌ Found 1 failing file with 6 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
iOS Safer C++ Build #18966 (4566606)❌ Found 1 failing file with 6 issues. Please address these issues before landing. See WebKit Guidelines for Safer C++ Programming. |
4566606 to
aee4c24
Compare
|
EWS run on previous version of this PR (hash aee4c24) Details |
|
|
||
| // Function-kind only. Lazy-install under m_ipintBytecodeLock; buffer is | ||
| // immutable once published. | ||
| Ref<const IPIntSharedBytecode> ensureArgumINTBytecode(const CallInformation&) const; |
There was a problem hiding this comment.
I don't think you use the results of these ensureXYZBytecode() functions anywhere. Could probably avoid a ref count churn.
There was a problem hiding this comment.
Sounds good. Changed.
| if (auto buffer = m_argumINTBytecode) [[likely]] | ||
| return buffer.releaseNonNull(); |
There was a problem hiding this comment.
Why not do the:
if (m_argumINTBytecode) [[likely]]
return;
Locker ...
if (m_argumINTBytecode)
return;
idiom?
There was a problem hiding this comment.
Sounds good. Changed.
| SUPPRESS_UNCOUNTED_LOCAL const auto& rtt = *call->signature.rtt; | ||
| rtt.ensureCallBytecode(); | ||
|
|
||
| auto* callee = IPINT_CALLEE(callFrame); |
There was a problem hiding this comment.
All this code seems heavily duplicated after the rtt.ensureXYZBytecode() can we have a shared helper?
There was a problem hiding this comment.
Sounds good. Changed.
aee4c24 to
834d881
Compare
|
EWS run on previous version of this PR (hash 834d881) Details
|
834d881 to
8811767
Compare
|
EWS run on previous version of this PR (hash 8811767) Details
|
8811767 to
af5ffec
Compare
|
EWS run on previous version of this PR (hash af5ffec) Details
|
af5ffec to
da03041
Compare
|
EWS run on previous version of this PR (hash da03041) Details
|
da03041 to
3bc3af1
Compare
|
EWS run on previous version of this PR (hash 3bc3af1) Details
|
kmiller68
left a comment
There was a problem hiding this comment.
Discussing offline, I think there's a bug here because there's no data dependency between the bytecode pointer and the other fields. Since the bytecode pointer controls publication it's possible for one of the other fields to get hoisted by a "reader" thread above the publication.
The latest version of the code fixed it by embedding that field into the bytecode :) |
3bc3af1 to
06c78ed
Compare
|
EWS run on previous version of this PR (hash 06c78ed) Details |
There was a problem hiding this comment.
Why is this refcounted rather than unique_ptr?
There was a problem hiding this comment.
Originally trying to share even further between some RTTs, like func(array, array) and func(struct, struct), but just going to the simpler way for now. Changed it to unique-ptr.
06c78ed to
bb3ff1d
Compare
|
EWS run on current version of this PR (hash bb3ff1d) Details |
https://bugs.webkit.org/show_bug.cgi?id=314133 rdar://176306252 Reviewed by Keith Miller. We move aINT / mINT / uINT bytecode to RTT since they are tied to the signature. This allows us to share them between calls with the same signature, and also this offers an ability to lazily generate it when mINT is actually used. 1. aINT / uINT are right now generated eagerly. This is similar to what we have right now. 2. mINT is generated when a particular callsite encounters RTT and there is no already registered bytecode. This becomes lazy generation, and not happening until the callsite is actually used. We save and restore MC in an unused slot before calls, which allows to switch MC to pointing at shared bytecode instead of IPInt metadata. We still have local bytecode which is used to initialize locals, but it should be improved subsequently. * Source/JavaScriptCore/llint/InPlaceInterpreter.asm: * Source/JavaScriptCore/llint/InPlaceInterpreter64.asm: * Source/JavaScriptCore/wasm/WasmCallee.cpp: (JSC::Wasm::IPIntCallee::IPIntCallee): * Source/JavaScriptCore/wasm/WasmCallee.h: * Source/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.cpp: (JSC::Wasm::FunctionIPIntMetadataGenerator::addReturnData): Deleted. * Source/JavaScriptCore/wasm/WasmFunctionIPIntMetadataGenerator.h: * Source/JavaScriptCore/wasm/WasmIPIntGenerator.cpp: (JSC::Wasm::IPIntGenerator::getCurrentInstructionLength): (JSC::Wasm::IPIntGenerator::cachedCallInformationFor): (JSC::Wasm::IPIntGenerator::addArguments): (JSC::Wasm::IPIntGenerator::addLocal): (JSC::Wasm::IPIntGenerator::addTailCallCommonData): (JSC::Wasm::IPIntGenerator::addCall): (JSC::Wasm::IPIntGenerator::addCallIndirect): (JSC::Wasm::IPIntGenerator::addCallRef): (JSC::Wasm::IPIntGenerator::finalize): (JSC::Wasm::addCallArgumentBytecode): Deleted. (JSC::Wasm::addCallResultBytecode): Deleted. (JSC::Wasm::IPIntGenerator::addCallCommonData): Deleted. * Source/JavaScriptCore/wasm/WasmIPIntGenerator.h: * Source/JavaScriptCore/wasm/WasmIPIntPlan.cpp: (JSC::Wasm::IPIntPlan::compileFunction): * Source/JavaScriptCore/wasm/WasmIPIntSlowPaths.cpp: (JSC::IPInt::ensureCallBytecodeForKind): (JSC::IPInt::prepareCallImpl): (JSC::IPInt::prepareCallIndirectImpl): (JSC::IPInt::prepareCallRefImpl): (JSC::IPInt::WASM_IPINT_EXTERN_CPP_DECL): * Source/JavaScriptCore/wasm/WasmIPIntSlowPaths.h: * Source/JavaScriptCore/wasm/WasmTypeDefinition.cpp: (JSC::Wasm::RTT::ensureArgumINTBytecode const): (JSC::Wasm::RTT::ensureUINTBytecode const): (JSC::Wasm::buildCallArgumentBytecode): (JSC::Wasm::buildCallResultBytecode): (JSC::Wasm::RTT::ensureCallBytecode const): (JSC::Wasm::RTT::ensureTailCallBytecode const): * Source/JavaScriptCore/wasm/WasmTypeDefinition.h: Canonical link: https://commits.webkit.org/312841@main
bb3ff1d to
5accc20
Compare
|
Committed 312841@main (5accc20): https://commits.webkit.org/312841@main Reviewed commits have been landed. Closing PR #64302 and removing active labels. |
🧪 bindings
5accc20
bb3ff1d
🛠 win🧪 win-tests🧪 ios-wk2-wpt🧪 api-mac-debug🧪 mac-intel-wk2