Bug 101328 – MIPS DFG implementation.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Balazs Kilvady]

DFG implementation for MIPS.

[Balazs Kilvady]

Created attachment 172540 [details]
DFG for MIPS

[Early Warning System Bot]

Comment on attachment 172540 [details]
DFG for MIPS

Attachment 172540 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14675417

[Early Warning System Bot]

Comment on attachment 172540 [details]
DFG for MIPS

Attachment 172540 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14732761

[EFL EWS Bot]

Comment on attachment 172540 [details]
DFG for MIPS

Attachment 172540 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14747377

[Build Bot]

Comment on attachment 172540 [details]
DFG for MIPS

Attachment 172540 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14726908

[Balazs Kilvady]

Created attachment 172763 [details]
DFG for MIPS.

Macro fix to make compilable on non-MIPS targets.

[Balazs Kilvady]

Created attachment 173088 [details]
DFG for MIPS.

Rebased.

[Chao-ying Fu]

 184    static const FPRReg argumentFPR0 = MIPSRegisters::f12;
 185    static const FPRReg argumentFPR1 = MIPSRegisters::f14;
 186    static const FPRReg argumentFPR2 = MIPSRegisters::f10; <----
 187    static const FPRReg argumentFPR3 = MIPSRegisters::f18;

I think at line 186 it should be MIPSRegisters::f16.
Are argumentFPR2 and argumentFPR3 used anywhere?
Thanks a lot!

[Balazs Kilvady]

Created attachment 174190 [details]
DFG for MIPS.

[Early Warning System Bot]

Comment on attachment 174190 [details]
DFG for MIPS.

Attachment 174190 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14844138

[EFL EWS Bot]

Comment on attachment 174190 [details]
DFG for MIPS.

Attachment 174190 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14844143

[Balazs Kilvady]

Created attachment 174198 [details]
DFG for MIPS.

[Balazs Kilvady]

Comment on attachment 174198 [details]
DFG for MIPS.

Rebased on r134612

[Balazs Kilvady]

Created attachment 179915 [details]
DFG for MIPS.

[Balazs Kilvady]

Comment on attachment 179915 [details]
DFG for MIPS.

Rebased on r138003.

[Balazs Kilvady]

Created attachment 180494 [details]
DFG for MIPS.

replaceWithJump problem (described in Bug #103146) fix added.

[Balazs Kilvady]

Created attachment 181319 [details]
DFG for MIPS.

[Balazs Kilvady]

Comment on attachment 181319 [details]
DFG for MIPS.

Rebased on r138804.

[Filip Pizlo]

Comment on attachment 181319 [details]
DFG for MIPS.

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

> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:963
> +#if CPU(MIPS)
> +    ALWAYS_INLINE void setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, GPRReg arg3, GPRReg arg4)
> +    {
> +        poke(arg4, 4);
> +        setupArgumentsWithExecState(arg1, arg2, arg3);
> +    }
> +

Why is all of this behind CPU(MIPS)?  It looks identical to the normal NUMBER_OF_ARGUMENT_REGISTERS == 4 case. :-/

[Balazs Kilvady]

(In reply to comment #19)
> (From update of attachment 181319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181319&action=review
> 
> > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:963
> > +#if CPU(MIPS)
> > +    ALWAYS_INLINE void setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, GPRReg arg3, GPRReg arg4)
> > +    {
> > +        poke(arg4, 4);
> > +        setupArgumentsWithExecState(arg1, arg2, arg3);
> > +    }
> > +
> 
> Why is all of this behind CPU(MIPS)?  It looks identical to the normal NUMBER_OF_ARGUMENT_REGISTERS == 4 case. :-/

Thank you for the review. On MIPS we have to give space for the register stored arguments on stack also so while other (params in 4 regs) arches can use:
        poke(arg5, 1);
        poke(arg4);
        setupArgumentsWithExecState(arg1, arg2, arg3);
on MIPS we have to use:
        poke(arg5, 5);
        poke(arg4, 4);
        setupArgumentsWithExecState(arg1, arg2, arg3);
Different stack offset values in poke call.

[Filip Pizlo]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 181319 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=181319&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGCCallHelpers.h:963
> > > +#if CPU(MIPS)
> > > +    ALWAYS_INLINE void setupArgumentsWithExecState(GPRReg arg1, GPRReg arg2, GPRReg arg3, GPRReg arg4)
> > > +    {
> > > +        poke(arg4, 4);
> > > +        setupArgumentsWithExecState(arg1, arg2, arg3);
> > > +    }
> > > +
> > 
> > Why is all of this behind CPU(MIPS)?  It looks identical to the normal NUMBER_OF_ARGUMENT_REGISTERS == 4 case. :-/
> 
> Thank you for the review. On MIPS we have to give space for the register stored arguments on stack also so while other (params in 4 regs) arches can use:
>         poke(arg5, 1);
>         poke(arg4);
>         setupArgumentsWithExecState(arg1, arg2, arg3);
> on MIPS we have to use:
>         poke(arg5, 5);
>         poke(arg4, 4);
>         setupArgumentsWithExecState(arg1, arg2, arg3);
> Different stack offset values in poke call.

Then I would suggest abstracting the poke offset, and using #if's only for that.  That way, you don't have to duplicate all of that code!

Keep in mind that this code gets churned *a lot*.  Every time we add a DFGOperations function with a sufficiently exotic signature, this code gets touched.  Therefore, we should reduce code duplication as much as possible; otherwise you guys will have a lot more work to do to keep up with DFG changes.

[Filip Pizlo]

Comment on attachment 181319 [details]
DFG for MIPS.

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

>>>> Source/JavaScriptCore/dfg/DFGCCallHelpers.h:963
>>>> +
>>> 
>>> Why is all of this behind CPU(MIPS)?  It looks identical to the normal NUMBER_OF_ARGUMENT_REGISTERS == 4 case. :-/
>> 
>> Thank you for the review. On MIPS we have to give space for the register stored arguments on stack also so while other (params in 4 regs) arches can use:
>>         poke(arg5, 1);
>>         poke(arg4);
>>         setupArgumentsWithExecState(arg1, arg2, arg3);
>> on MIPS we have to use:
>>         poke(arg5, 5);
>>         poke(arg4, 4);
>>         setupArgumentsWithExecState(arg1, arg2, arg3);
>> Different stack offset values in poke call.
> 
> Then I would suggest abstracting the poke offset, and using #if's only for that.  That way, you don't have to duplicate all of that code!
> 
> Keep in mind that this code gets churned *a lot*.  Every time we add a DFGOperations function with a sufficiently exotic signature, this code gets touched.  Therefore, we should reduce code duplication as much as possible; otherwise you guys will have a lot more work to do to keep up with DFG changes.

See above comment.  I still think you should abstract the poke offset rather than duplicating all of this code.

[Filip Pizlo]

Comment on attachment 179915 [details]
DFG for MIPS.

I assume that this is an earlier version of the patch, so I'm clearing r?.

[Balazs Kilvady]

Created attachment 181695 [details]
DFG for MIPS.

With the requested poke abstraction which is implemented with a helper inline template function.

[Balazs Kilvady]

Created attachment 181698 [details]
DFG for MIPS #2.

With the requested poke abstraction which is implemented with an offset macro.

[Balazs Kilvady]

(In reply to comment #22)
> See above comment.  I still think you should abstract the poke offset rather than duplicating all of this code.

Thank you for reviewing. We found two "straight" ways for poke abstraction but we were unsure which solution you would like, so we provided both in the last two patches. If you would like to see something else, please let us know, and we will of course change the code accordingly.

[Balazs Kilvady]

Created attachment 183415 [details]
DFG for MIPS - arg. offset with macro

[Balazs Kilvady]

Created attachment 183416 [details]
DFG for MIPS - arg. offset with inline

[Balazs Kilvady]

Created attachment 183746 [details]
DFG for MIPS - arg. offset with inline

[Balazs Kilvady]

Created attachment 183750 [details]
DFG for MIPS - arg. offset with macro

Rebased on r140318.

[Balazs Kilvady]

Created attachment 185217 [details]
DFG for MIPS - arg. offset with inline

Rebased on r 41096.

[Balazs Kilvady]

Created attachment 185219 [details]
DFG for MIPS - arg. offset with macro

Rebased on r141096.

[Balazs Kilvady]

Comment on attachment 185217 [details]
DFG for MIPS - arg. offset with inline

Rebased on r141096.

[Balazs Kilvady]

Created attachment 187616 [details]
DFG for MIPS - arg. offset with inline

Rebased on r142489.

[Balazs Kilvady]

Created attachment 188878 [details]
DFG for MIPS - arg. offset with inline

Rebased on r143211.

[Balazs Kilvady]

Created attachment 188879 [details]
DFG for MIPS - arg. offset with macro

Rebased on r143211.

[WebKit Review Bot]

Comment on attachment 188879 [details]
DFG for MIPS - arg. offset with macro

Clearing flags on attachment: 188879

Committed r143247: <http://trac.webkit.org/changeset/143247>

[Zan Dobersek]

Comment on attachment 188878 [details]
DFG for MIPS - arg. offset with inline

The work on this bug has finished, with the patch with macro-based approach landed in comment #37.

Clearing the flags on this parallel patch that was based on the inline functions approach.