[Webkit-unassigned] [Bug 222903] [WASM-Function-References] Add call_ref instruction
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 19 16:07:02 PDT 2021
https://bugs.webkit.org/show_bug.cgi?id=222903
Yusuke Suzuki <ysuzuki at apple.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #423144|review?, commit-queue? |review-, commit-queue-
Flags| |
--- Comment #8 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 423144
--> https://bugs.webkit.org/attachment.cgi?id=423144
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=423144&action=review
The code looks good, but I think we should dedupe the code.
> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:3509
> + // Do a context switch if needed.
> + {
> + auto targetInstance = g64();
> + append(Move, Arg::addr(calleeFunction, WebAssemblyFunctionBase::offsetOfInstance()), targetInstance);
> +
> + BasicBlock* doContextSwitch = m_code.addBlock();
> + BasicBlock* continuation = m_code.addBlock();
> +
> + append(Branch64, Arg::relCond(MacroAssembler::Equal), targetInstance, instanceValue());
> + m_currentBlock->setSuccessors(continuation, doContextSwitch);
> +
> + auto* patchpoint = addPatchpoint(B3::Void);
> + patchpoint->effects.writesPinned = true;
> + // We pessimistically assume we're calling something with BoundsChecking memory.
> + // FIXME: We shouldn't have to do this: https://bugs.webkit.org/show_bug.cgi?id=172181
> + patchpoint->clobber(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> + patchpoint->clobber(RegisterSet::macroScratchRegisters());
> + patchpoint->numGPScratchRegisters = Gigacage::isEnabled(Gigacage::Primitive) ? 1 : 0;
> +
> + patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> + AllowMacroScratchRegisterUsage allowScratch(jit);
> + GPRReg targetInstance = params[0].gpr();
> + GPRReg oldContextInstance = params[1].gpr();
> + const PinnedRegisterInfo& pinnedRegs = PinnedRegisterInfo::get();
> + GPRReg baseMemory = pinnedRegs.baseMemoryPointer;
> + ASSERT(targetInstance != baseMemory);
> + jit.loadPtr(CCallHelpers::Address(oldContextInstance, Instance::offsetOfCachedStackLimit()), baseMemory);
> + jit.storePtr(baseMemory, CCallHelpers::Address(targetInstance, Instance::offsetOfCachedStackLimit()));
> + jit.storeWasmContextInstance(targetInstance);
> + // FIXME: We should support more than one memory size register
> + // see: https://bugs.webkit.org/show_bug.cgi?id=162952
> + ASSERT(pinnedRegs.boundsCheckingSizeRegister != targetInstance);
> + GPRReg scratchOrBoundsCheckingSize = Gigacage::isEnabled(Gigacage::Primitive) ? params.gpScratch(0) : pinnedRegs.boundsCheckingSizeRegister;
> +
> + jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedBoundsCheckingSize()), pinnedRegs.boundsCheckingSizeRegister); // Bound checking size.
> + jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedMemory()), baseMemory); // Memory::void*.
> +
> + jit.cageConditionally(Gigacage::Primitive, baseMemory, pinnedRegs.boundsCheckingSizeRegister, scratchOrBoundsCheckingSize);
> + });
> +
> + emitPatchpoint(doContextSwitch, patchpoint, Tmp(), targetInstance, instanceValue());
> + append(doContextSwitch, Jump);
> + doContextSwitch->setSuccessors(continuation);
> +
> + m_currentBlock = continuation;
> + }
> +
> + append(Move, Arg::addr(calleeCode), calleeCode);
> +
> + Vector<ConstrainedTmp> extraArgs;
> + extraArgs.append(calleeCode);
> +
> + for (unsigned i = 0; i < signature.returnCount(); ++i)
> + results.append(tmpForType(signature.returnType(i)));
> +
> + auto* patchpoint = emitCallPatchpoint(m_currentBlock, signature, results, args, WTFMove(extraArgs));
> +
> + // We need to clobber all potential pinned registers since we might be leaving the instance.
> + // We pessimistically assume we're always calling something that is bounds checking so
> + // because the wasm->wasm thunk unconditionally overrides the size registers.
> + // FIXME: We should not have to do this, but the wasm->wasm stub assumes it can
> + // use all the pinned registers as scratch: https://bugs.webkit.org/show_bug.cgi?id=172181
> +
> + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> +
> + patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> + AllowMacroScratchRegisterUsage allowScratch(jit);
> + jit.call(params[params.proc().resultCount(params.value()->type())].gpr(), WasmEntryPtrTag);
> + });
> +
> + // The call could have been to another WebAssembly instance, and / or could have modified our Memory.
> + restoreWebAssemblyGlobalState(RestoreCachedStackLimit::Yes, m_info.memory, currentInstance, m_currentBlock);
> +
> + return { };
I think large part of this code is the same to addCallIndirect one, can you dedupe them by extracting this part?
> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2722
> + // Do a context switch if needed.
> + {
> + Value* offset = constant(pointerType(), safeCast<int32_t>(WebAssemblyFunctionBase::offsetOfInstance()));
> + Value* targetInstance = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(),
> + m_currentBlock->appendNew<Value>(m_proc, Add, origin(), callableFunction, offset));
> + BasicBlock* continuation = m_proc.addBlock();
> + BasicBlock* doContextSwitch = m_proc.addBlock();
> +
> + Value* isSameContextInstance = m_currentBlock->appendNew<Value>(m_proc, Equal, origin(),
> + targetInstance, instanceValue());
> + m_currentBlock->appendNewControlValue(m_proc, B3::Branch, origin(),
> + isSameContextInstance, FrequentedBlock(continuation), FrequentedBlock(doContextSwitch));
> +
> + PatchpointValue* patchpoint = doContextSwitch->appendNew<PatchpointValue>(m_proc, B3::Void, origin());
> + patchpoint->effects.writesPinned = true;
> + // We pessimistically assume we're calling something with BoundsChecking memory.
> + // FIXME: We shouldn't have to do this: https://bugs.webkit.org/show_bug.cgi?id=172181
> + patchpoint->clobber(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> + patchpoint->clobber(RegisterSet::macroScratchRegisters());
> + patchpoint->append(targetInstance, ValueRep::SomeRegister);
> + patchpoint->append(instanceValue(), ValueRep::SomeRegister);
> + patchpoint->numGPScratchRegisters = Gigacage::isEnabled(Gigacage::Primitive) ? 1 : 0;
> +
> + patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> + AllowMacroScratchRegisterUsage allowScratch(jit);
> + GPRReg targetInstance = params[0].gpr();
> + GPRReg oldContextInstance = params[1].gpr();
> + const PinnedRegisterInfo& pinnedRegs = PinnedRegisterInfo::get();
> + GPRReg baseMemory = pinnedRegs.baseMemoryPointer;
> + ASSERT(targetInstance != baseMemory);
> + jit.loadPtr(CCallHelpers::Address(oldContextInstance, Instance::offsetOfCachedStackLimit()), baseMemory);
> + jit.storePtr(baseMemory, CCallHelpers::Address(targetInstance, Instance::offsetOfCachedStackLimit()));
> + jit.storeWasmContextInstance(targetInstance);
> + ASSERT(pinnedRegs.boundsCheckingSizeRegister != baseMemory);
> + // FIXME: We should support more than one memory size register
> + // see: https://bugs.webkit.org/show_bug.cgi?id=162952
> + ASSERT(pinnedRegs.boundsCheckingSizeRegister != targetInstance);
> + GPRReg scratchOrBoundsCheckingSize = Gigacage::isEnabled(Gigacage::Primitive) ? params.gpScratch(0) : pinnedRegs.boundsCheckingSizeRegister;
> +
> + jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedBoundsCheckingSize()), pinnedRegs.boundsCheckingSizeRegister); // Memory size.
> + jit.loadPtr(CCallHelpers::Address(targetInstance, Instance::offsetOfCachedMemory()), baseMemory); // Memory::void*.
> +
> + jit.cageConditionally(Gigacage::Primitive, baseMemory, pinnedRegs.boundsCheckingSizeRegister, scratchOrBoundsCheckingSize);
> + });
> + doContextSwitch->appendNewControlValue(m_proc, Jump, origin(), continuation);
> +
> + m_currentBlock = continuation;
> + }
> +
> + ExpressionType calleeCode = m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(),
> + m_currentBlock->appendNew<MemoryValue>(m_proc, Load, pointerType(), origin(), callableFunction,
> + safeCast<int32_t>(WebAssemblyFunction::offsetOfEntrypointLoadLocation())));
> +
> + B3::Type returnType = toB3ResultType(&signature);
> + ExpressionType callResult = createCallPatchpoint(m_currentBlock, origin(), signature, args,
> + scopedLambdaRef<void(PatchpointValue*)>([=] (PatchpointValue* patchpoint) -> void {
> + patchpoint->effects.writesPinned = true;
> + patchpoint->effects.readsPinned = true;
> + // We need to clobber all potential pinned registers since we might be leaving the instance.
> + // We pessimistically assume we're always calling something that is bounds checking so
> + // because the wasm->wasm thunk unconditionally overrides the size registers.
> + // FIXME: We should not have to do this, but the wasm->wasm stub assumes it can
> + // use all the pinned registers as scratch: https://bugs.webkit.org/show_bug.cgi?id=172181
> + patchpoint->clobberLate(PinnedRegisterInfo::get().toSave(MemoryMode::BoundsChecking));
> +
> + patchpoint->append(calleeCode, ValueRep::SomeRegister);
> + patchpoint->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams& params) {
> + AllowMacroScratchRegisterUsage allowScratch(jit);
> + jit.call(params[params.proc().resultCount(returnType)].gpr(), WasmEntryPtrTag);
> + });
> + }));
> +
> + switch (returnType.kind()) {
> + case B3::Void: {
> + break;
> + }
> + case B3::Tuple: {
> + const Vector<B3::Type>& tuple = m_proc.tupleForType(returnType);
> + for (unsigned i = 0; i < signature.returnCount(); ++i)
> + results.append(m_currentBlock->appendNew<ExtractValue>(m_proc, origin(), tuple[i], callResult, i));
> + break;
> + }
> + default: {
> + results.append(callResult);
> + break;
> + }
> + }
> +
> + // The call could have been to another WebAssembly instance, and / or could have modified our Memory.
> + restoreWebAssemblyGlobalState(RestoreCachedStackLimit::Yes, m_info.memory, instanceValue(), m_proc, m_currentBlock);
> +
> + return { };
> +}
I think large part of this is duplicate to addCallIndirect. Can you extract this and dedupe the code?
> Source/JavaScriptCore/wasm/WasmFunctionParser.h:1255
> + const SignatureIndex calleeSignatureIndex = reinterpret_cast<SignatureIndex>(m_expressionStack.last().type().index);
We do not need to have reinterpret_cast since Type::index is SignatureIndex
> Source/JavaScriptCore/wasm/WasmSlowPaths.cpp:518
> + WebAssemblyFunction* wasmFunction = jsCast<WebAssemblyFunction*>(referenceAsObject);
> + Wasm::Instance* targetInstance = &wasmFunction->instance()->instance();
> +
> + if (targetInstance != instance)
> + targetInstance->setCachedStackLimit(instance->cachedStackLimit());
> +
> + const Wasm::WasmToWasmImportableFunction& function = wasmFunction->importableFunction();
> + WASM_CALL_RETURN(targetInstance, function.entrypointLoadLocation->executableAddress(), WasmEntryPtrTag);
Why don't we need to perform signature check here? If it is not necessary, can you insert ASSERT that ensures the signature is correct?
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210319/79d3a74a/attachment-0001.htm>
More information about the webkit-unassigned
mailing list