[webkit-reviews] review denied: [Bug 222903] [WASM-Function-References] Add call_ref instruction : [Attachment 423144] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 19 16:07:02 PDT 2021
Yusuke Suzuki <ysuzuki at apple.com> has denied Dmitry <dbezhetskov at igalia.com>'s
request for review:
Bug 222903: [WASM-Function-References] Add call_ref instruction
https://bugs.webkit.org/show_bug.cgi?id=222903
Attachment 423144: Patch
https://bugs.webkit.org/attachment.cgi?id=423144&action=review
--- 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::BoundsChec
king));
> +
> + 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::BoundsChec
king));
> +
> + 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?
More information about the webkit-reviews
mailing list