[webkit-reviews] review granted: [Bug 183786] Implement setupArgumentsImpl for ARM and MIPS : [Attachment 338093] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has granted Dominik Inführ
<dinfuehr at igalia.com>'s request for review:
Bug 183786: Implement setupArgumentsImpl for ARM and MIPS
https://bugs.webkit.org/show_bug.cgi?id=183786

Attachment 338093: Patch

https://bugs.webkit.org/attachment.cgi?id=338093&action=review




--- Comment #15 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 338093
  --> https://bugs.webkit.org/attachment.cgi?id=338093
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338093&action=review

r=me with nits. Nice work!

> Source/JavaScriptCore/ChangeLog:39
> +	   * assembler/MacroAssemblerARMv7.h:
> +	   (JSC::MacroAssemblerARMv7::moveDouble):
> +	   * assembler/MacroAssemblerMIPS.h:
> +	   (JSC::MacroAssemblerMIPS::moveDouble):
> +	   * jit/CCallHelpers.h:
> +	   (JSC::CCallHelpers::setupStubCrossArgs):
> +	   (JSC::CCallHelpers::ArgCollection::ArgCollection):
> +	   (JSC::CCallHelpers::ArgCollection::pushRegArg):
> +	   (JSC::CCallHelpers::ArgCollection::pushExtraRegArg):
> +	   (JSC::CCallHelpers::ArgCollection::addGPRArg):
> +	   (JSC::CCallHelpers::ArgCollection::addGPRExtraArg):
> +	   (JSC::CCallHelpers::ArgCollection::addStackArg):
> +	   (JSC::CCallHelpers::ArgCollection::addPoke):
> +	   (JSC::CCallHelpers::ArgCollection::argCount):
> +	   (JSC::CCallHelpers::clampArrayToSize):
> +	   (JSC::CCallHelpers::calculatePokeOffset):
> +	   (JSC::CCallHelpers::pokeForArgument):
> +	   (JSC::CCallHelpers::pokeAligned):
> +	   (JSC::CCallHelpers::marshallArgumentRegister):
> +	   (JSC::CCallHelpers::setupArgumentsImpl):
> +	   (JSC::CCallHelpers::align2):
> +	   (JSC::CCallHelpers::pokeArgumentsAligned):
> +	   (JSC::CCallHelpers::std::is_integral<CURRENT_ARGUMENT_TYPE>::value):
> +	   (JSC::CCallHelpers::std::is_pointer<CURRENT_ARGUMENT_TYPE>::value):
> +	   (JSC::CCallHelpers::setupArguments):
> +	   * jit/FPRInfo.h:
> +	   (JSC::FPRInfo::toArgumentRegister):

Could you update this ChangeLog to reflect the change in the latest patch? You
can run `Tools/Scripts/webkit-patch upload --update-changelogs`.

> Source/JavaScriptCore/jit/CCallHelpers.h:585
> +	       auto updatedArgSourceRegs1 =
argSourceRegs.pushRegArg(arg.payloadGPR(),
GPRInfo::toArgumentRegister(alignedArgCount));
> +	       auto updatedArgSourceRegs2 =
updatedArgSourceRegs1.pushExtraRegArg(arg.tagGPR(),
GPRInfo::toArgumentRegister(alignedArgCount + 1));

OK, so can you add a comment about why we are increasing extra reg arg and not
increasing numGPRArgs by 2 instead? It is tricky if we know that JSValueRegs is
represented by two GPRRegs in 32bit function calls.

> Source/JavaScriptCore/jit/CCallHelpers.h:674
> +#if CPU(JSVALUE64)
>	   static_assert(FunctionTraits<OperationType>::cCallArity() ==
numGPRArgs + numFPRArgs + extraPoke, "Check the CCall arity");
> +#endif

So, can we keep this static_assert for CPU(X86) too? It is useful for keeping
32bit build sane.