Bug 183786 – Implement setupArgumentsImpl for ARM and MIPS

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dominik Inführ]

Implement setupArgumentsImpl for ARM and MIPS

[Dominik Inführ]

Created attachment 336138 [details]
Patch

[Dominik Inführ]

Created attachment 336140 [details]
Patch

[Dominik Inführ]

I've fixed the unused parameter warning.

[Dominik Inführ]

Created attachment 336183 [details]
Patch

[Keith Miller]

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.

[Ross Kirsling]

(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.

[Dominik Inführ]

Created attachment 336644 [details]
Patch

[Dominik Inführ]

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.

[Guillaume Emont]

Ping reviewer. These platforms don't work at all without this patch.

[Carlos Alberto Lopez Perez]

(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.

[Yusuke Suzuki]

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.

[Dominik Inführ]

Created attachment 337996 [details]
Patch

[Dominik Inführ]

Created attachment 338093 [details]
Patch

[Dominik Inführ]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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.

[Dominik Inführ]

Created attachment 338115 [details]
Patch

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 338115 [details]
Patch

Clearing flags on attachment: 338115

Committed r230717: <https://trac.webkit.org/changeset/230717>

[Radar WebKit Bug Importer]

<rdar://problem/39968383>