Bug 101328

Summary: MIPS DFG implementation.
Product: WebKit Reporter: Balazs Kilvady <kilvadyb>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, cmarcelo, fpizlo, fu, gergely, gustavo, ojan.autocc, oliver, ossy, palfia, philn, webkit-ews, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 108664, 108662    
Attachments:
Description Flags
DFG for MIPS
webkit-ews: commit-queue-
DFG for MIPS.
none
DFG for MIPS.
none
DFG for MIPS.
webkit-ews: commit-queue-
DFG for MIPS.
none
DFG for MIPS.
none
DFG for MIPS.
none
DFG for MIPS.
fpizlo: review-, fpizlo: commit-queue-
DFG for MIPS.
none
DFG for MIPS #2.
none
DFG for MIPS - arg. offset with macro
none
DFG for MIPS - arg. offset with inline
none
DFG for MIPS - arg. offset with inline
none
DFG for MIPS - arg. offset with macro
none
DFG for MIPS - arg. offset with inline
none
DFG for MIPS - arg. offset with macro
none
DFG for MIPS - arg. offset with inline
none
DFG for MIPS - arg. offset with inline
none
DFG for MIPS - arg. offset with macro none

Description Balazs Kilvady 2012-11-06 02:47:50 PST
DFG implementation for MIPS.
Comment 1 Balazs Kilvady 2012-11-06 03:11:23 PST
Created attachment 172540 [details]
DFG for MIPS
Comment 2 Early Warning System Bot 2012-11-06 03:25:56 PST
Comment on attachment 172540 [details]
DFG for MIPS

Attachment 172540 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14675417
Comment 3 Early Warning System Bot 2012-11-06 03:29:09 PST
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
Comment 4 EFL EWS Bot 2012-11-06 04:19:58 PST
Comment on attachment 172540 [details]
DFG for MIPS

Attachment 172540 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14747377
Comment 5 Build Bot 2012-11-06 06:57:21 PST
Comment on attachment 172540 [details]
DFG for MIPS

Attachment 172540 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14726908
Comment 6 Balazs Kilvady 2012-11-07 05:14:50 PST
Created attachment 172763 [details]
DFG for MIPS.

Macro fix to make compilable on non-MIPS targets.
Comment 7 Balazs Kilvady 2012-11-08 12:04:15 PST
Created attachment 173088 [details]
DFG for MIPS.

Rebased.
Comment 8 Chao-ying Fu 2012-11-09 14:17:40 PST
 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!
Comment 9 Balazs Kilvady 2012-11-14 10:16:48 PST
Created attachment 174190 [details]
DFG for MIPS.
Comment 10 Early Warning System Bot 2012-11-14 10:25:09 PST
Comment on attachment 174190 [details]
DFG for MIPS.

Attachment 174190 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14844138
Comment 11 EFL EWS Bot 2012-11-14 10:35:02 PST
Comment on attachment 174190 [details]
DFG for MIPS.

Attachment 174190 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14844143
Comment 12 Balazs Kilvady 2012-11-14 10:35:42 PST
Created attachment 174198 [details]
DFG for MIPS.
Comment 13 Balazs Kilvady 2012-11-14 10:38:38 PST
Comment on attachment 174198 [details]
DFG for MIPS.

Rebased on r134612
Comment 14 Balazs Kilvady 2012-12-18 03:27:59 PST
Created attachment 179915 [details]
DFG for MIPS.
Comment 15 Balazs Kilvady 2012-12-18 03:30:11 PST
Comment on attachment 179915 [details]
DFG for MIPS.

Rebased on r138003.
Comment 16 Balazs Kilvady 2012-12-21 02:09:40 PST
Created attachment 180494 [details]
DFG for MIPS.

replaceWithJump problem (described in Bug #103146) fix added.
Comment 17 Balazs Kilvady 2013-01-04 09:38:24 PST
Created attachment 181319 [details]
DFG for MIPS.
Comment 18 Balazs Kilvady 2013-01-04 10:04:48 PST
Comment on attachment 181319 [details]
DFG for MIPS.

Rebased on r138804.
Comment 19 Filip Pizlo 2013-01-05 23:25:38 PST
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. :-/
Comment 20 Balazs Kilvady 2013-01-07 02:19:21 PST
(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.
Comment 21 Filip Pizlo 2013-01-07 10:54:44 PST
(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.
Comment 22 Filip Pizlo 2013-01-07 11:06:36 PST
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.
Comment 23 Filip Pizlo 2013-01-07 11:07:43 PST
Comment on attachment 179915 [details]
DFG for MIPS.

I assume that this is an earlier version of the patch, so I'm clearing r?.
Comment 24 Balazs Kilvady 2013-01-08 08:10:14 PST
Created attachment 181695 [details]
DFG for MIPS.

With the requested poke abstraction which is implemented with a helper inline template function.
Comment 25 Balazs Kilvady 2013-01-08 08:19:17 PST
Created attachment 181698 [details]
DFG for MIPS #2.

With the requested poke abstraction which is implemented with an offset macro.
Comment 26 Balazs Kilvady 2013-01-08 08:27:04 PST
(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.
Comment 27 Balazs Kilvady 2013-01-18 02:46:40 PST
Created attachment 183415 [details]
DFG for MIPS - arg. offset with macro
Comment 28 Balazs Kilvady 2013-01-18 02:55:35 PST
Created attachment 183416 [details]
DFG for MIPS - arg. offset with inline
Comment 29 Balazs Kilvady 2013-01-21 03:24:00 PST
Created attachment 183746 [details]
DFG for MIPS - arg. offset with inline
Comment 30 Balazs Kilvady 2013-01-21 03:36:21 PST
Created attachment 183750 [details]
DFG for MIPS - arg. offset with macro

Rebased on r140318.
Comment 31 Balazs Kilvady 2013-01-29 04:38:13 PST
Created attachment 185217 [details]
DFG for MIPS - arg. offset with inline

Rebased on r 41096.
Comment 32 Balazs Kilvady 2013-01-29 04:46:05 PST
Created attachment 185219 [details]
DFG for MIPS - arg. offset with macro

Rebased on r141096.
Comment 33 Balazs Kilvady 2013-01-29 04:47:16 PST
Comment on attachment 185217 [details]
DFG for MIPS - arg. offset with inline

Rebased on r141096.
Comment 34 Balazs Kilvady 2013-02-11 11:07:10 PST
Created attachment 187616 [details]
DFG for MIPS - arg. offset with inline

Rebased on r142489.
Comment 35 Balazs Kilvady 2013-02-18 07:04:04 PST
Created attachment 188878 [details]
DFG for MIPS - arg. offset with inline

Rebased on r143211.
Comment 36 Balazs Kilvady 2013-02-18 07:12:42 PST
Created attachment 188879 [details]
DFG for MIPS - arg. offset with macro

Rebased on r143211.
Comment 37 WebKit Review Bot 2013-02-18 11:27:04 PST
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>
Comment 38 Zan Dobersek 2013-09-05 08:49:41 PDT
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.