Implement setupArgumentsImpl for ARM and MIPS
Created attachment 336138 [details] Patch
Created attachment 336140 [details] Patch
I've fixed the unused parameter warning.
Created attachment 336183 [details] Patch
Comment on attachment 336183 [details] Patch After reading this I'm kinda starting to think that we should duplicate this code for each architecture via #ifs... With this change (and kinda before), you really need to deeply understand every architecture in order to understand the one you are interested in.
(In reply to Keith Miller from comment #5) > Comment on attachment 336183 [details] > Patch > > After reading this I'm kinda starting to think that we should duplicate this > code for each architecture via #ifs... With this change (and kinda before), > you really need to deeply understand every architecture in order to > understand the one you are interested in. +1 -- I think we'll need to do this for https://bugs.webkit.org/show_bug.cgi?id=183655 as well.
Created attachment 336644 [details] Patch
I've updated the patch. The patch now implements ARM (hardfp and softfp) and MIPS calling convention. x86 uses still its own `#if CPU(X86)`. I didn't split the implementation of ARM & MIPS though since they are quite similar and only differ in one function. But instead of having those GPRInfo:: variables, I am now using #ifs in this function. Hope you like that better.
Ping reviewer. These platforms don't work at all without this patch.
(In reply to Guillaume Emont from comment #9) > Ping reviewer. These platforms don't work at all without this patch. Second ping JSC reviewers? This bug is blocking on this platforms, please take a look if possible.
Comment on attachment 336644 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=336644&action=review > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:969 > + m_assembler.vmov(dest, RegisterID(dest+1), src); Use `dest + 1`. > Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:2926 > + m_assembler.mfc1(RegisterID(dest+1), FPRegisterID(src+1)); Ditto. > Source/JavaScriptCore/jit/CCallHelpers.h:305 > + RELEASE_ASSERT(TargetSize <= sourceArray.size()); Keep `static_assert`. > Source/JavaScriptCore/jit/CCallHelpers.h:341 > + ALWAYS_INLINE bool pokeAligned(unsigned currentGPRArgument, unsigned currentFPRArgument, unsigned numCrossSources, unsigned extraGPRArgs, unsigned extraPoke) The function name seems confusing. It does not `poke`. Use the name like `shouldPokeAligned` or the other. > Source/JavaScriptCore/jit/CCallHelpers.h:-337 > -#else // USE(JSVALUE64) Let's keep this #else. > Source/JavaScriptCore/jit/CCallHelpers.h:399 > +#elif CPU(X86) And add it as `#if CPU(X86)`. > Source/JavaScriptCore/jit/CCallHelpers.h:449 > + ALWAYS_INLINE unsigned align2(unsigned val) > + { > + return (val + 1) & (~1); > + } Use `roundUpToMultipleOf<2>(val)` instead and remove this function. > Source/JavaScriptCore/jit/CCallHelpers.h:493 > + pokeForArgument(arg, numGPRArgs, numFPRArgs, numCrossSources, extraGPRArgs+1, extraPoke); Use `extraPoke + 1`. > Source/JavaScriptCore/jit/CCallHelpers.h:503 > + pokeForArgument(arg, numGPRArgs, numFPRArgs, numCrossSources, extraGPRArgs, extraPoke+1); Use `extraPoke + 1`. > Source/JavaScriptCore/jit/CCallHelpers.h:534 > + Remove this blank line. > Source/JavaScriptCore/jit/CCallHelpers.h:539 > + Remove this blank line. > Source/JavaScriptCore/jit/CCallHelpers.h:549 > + setupArgumentsImpl(ArgCollection<numGPRArgs, numGPRSources, numFPRArgs, numFPRSources, numCrossSources, extraGPRArgs, extraPoke> argSourceRegs, JSValue::JSCellTag, GPRReg payload, Args... args) Now, `JSValue::JSCellTag` is removed. We are using `CCallHelpers::CellValue` instead. > Source/JavaScriptCore/jit/CCallHelpers.h:563 > + move(TrustedImm32(JSValue::CellTag), GPRInfo::toArgumentRegister(alignedArgCount+1)); Use `alignedArgCount + 1`. > Source/JavaScriptCore/jit/CCallHelpers.h:564 > + Remove this blank line. > Source/JavaScriptCore/jit/CCallHelpers.h:571 > + setupArgumentsImpl(ArgCollection<numGPRArgs, numGPRSources, numFPRArgs, numFPRSources, numCrossSources, extraGPRArgs, extraPoke> argSourceRegs, JSValue::JSCellTag, TrustedImmPtr payload, Args... args) Ditto. But maybe this function is not necessary. We do not have `JSValue::JSCellTag & TrustedImmPtr` combination right now. > Source/JavaScriptCore/jit/CCallHelpers.h:584 > + move(TrustedImm32(JSValue::CellTag), GPRInfo::toArgumentRegister(alignedArgCount+1)); Ditto. > Source/JavaScriptCore/jit/CCallHelpers.h:600 > + auto updatedArgSourceRegs2 = updatedArgSourceRegs1.pushExtraRegArg(arg.tagGPR(), GPRInfo::toArgumentRegister(alignedArgCount+1)); Use `alignedArgCount + 1`. And is `pushExtraRegArg` correct? Do we always need extra reg arg to push JSValueRegs? > Source/JavaScriptCore/jit/CCallHelpers.h:606 > + Remove this blank line. > Source/JavaScriptCore/jit/CCallHelpers.h:-453 > -#undef RESULT_TYPE Do not remove this undef. > Source/JavaScriptCore/jit/CCallHelpers.h:688 > +#if CPU(JSVALUE64) > static_assert(FunctionTraits<OperationType>::cCallArity() == numGPRArgs + numFPRArgs + extraPoke, "Check the CCall arity"); > +#endif Add a valid static_assert for 32bit.
Created attachment 337996 [details] Patch
Created attachment 338093 [details] Patch
Thanks a lot for the review! I hope I've addressed the points you raised. The patch is now rebased/updated, it now uses CellValue. I didn't add a static_assert for 32-bit architectures though: Rules are a bit complicated here, I guess there is only a simple to write such a line after adding even more template parameters (to keep track of registers or stack slots not used due to padding). > And is `pushExtraRegArg` correct? Do we always need extra reg arg to push JSalueRegs? I think so, JSValueRegs is 64-bit and on ARM/MIPS such a value is passed in two 32-bit registers if possible. I can't increase numGPRArgs by 2 for that (since it is also used for CURRENT_ARGUMEN_TYPE, we would skip an argument). That's why I've added extraGPRArgs. Passing JSValueRegs in registers therefore means: increasing both numGPRARgs and extraGPRArgs by 1. If we need to skip a register for "alignment" (64-bit value can only start in register r0/r1 or r2/r3 but not r1/r2), extraGPRArgs even needs to be increased by 2.
Comment on attachment 338093 [details] 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.
Comment on attachment 338093 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338093&action=review >> Source/JavaScriptCore/ChangeLog:39 >> + (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`. If you want not to clear r+, you can use like `Tools/Scripts/webkit-patch upload --update-changelogs --no-review --no-obsolete`. And you can set cq? for that patch.
Created attachment 338115 [details] Patch
Comment on attachment 338115 [details] Patch Rejecting attachment 338115 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 338115, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/7344653
Comment on attachment 338115 [details] Patch Clearing flags on attachment: 338115 Committed r230717: <https://trac.webkit.org/changeset/230717>
<rdar://problem/39968383>