Bug 101328 - MIPS DFG implementation.
: MIPS DFG implementation.
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Other Linux
: P2 Normal
Assigned To:
:
:
:
: 108662 108664
  Show dependency treegraph
 
Reported: 2012-11-06 02:47 PST by
Modified: 2013-09-05 08:49 PST (History)


Attachments
DFG for MIPS (44.79 KB, patch)
2012-11-06 03:11 PST, Balazs Kilvady
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (44.76 KB, patch)
2012-11-07 05:14 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (44.83 KB, patch)
2012-11-08 12:04 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (46.88 KB, patch)
2012-11-14 10:16 PST, Balazs Kilvady
webkit-ews: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (46.91 KB, patch)
2012-11-14 10:35 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (50.05 KB, patch)
2012-12-18 03:27 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (50.48 KB, patch)
2012-12-21 02:09 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (50.62 KB, patch)
2013-01-04 09:38 PST, Balazs Kilvady
fpizlo: review-
fpizlo: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
DFG for MIPS. (51.23 KB, patch)
2013-01-08 08:10 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS #2. (51.44 KB, patch)
2013-01-08 08:19 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with macro (51.46 KB, patch)
2013-01-18 02:46 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with inline (51.25 KB, patch)
2013-01-18 02:55 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with inline (51.19 KB, patch)
2013-01-21 03:24 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with macro (51.40 KB, patch)
2013-01-21 03:36 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with inline (51.24 KB, patch)
2013-01-29 04:38 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with macro (51.46 KB, patch)
2013-01-29 04:46 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with inline (51.24 KB, patch)
2013-02-11 11:07 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with inline (51.24 KB, patch)
2013-02-18 07:04 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff
DFG for MIPS - arg. offset with macro (51.45 KB, patch)
2013-02-18 07:12 PST, Balazs Kilvady
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-11-06 02:47:50 PST
DFG implementation for MIPS.
------- Comment #1 From 2012-11-06 03:11:23 PST -------
Created an attachment (id=172540) [details]
DFG for MIPS
------- Comment #2 From 2012-11-06 03:25:56 PST -------
(From update of attachment 172540 [details])
Attachment 172540 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14675417
------- Comment #3 From 2012-11-06 03:29:09 PST -------
(From update of attachment 172540 [details])
Attachment 172540 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14732761
------- Comment #4 From 2012-11-06 04:19:58 PST -------
(From update of attachment 172540 [details])
Attachment 172540 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14747377
------- Comment #5 From 2012-11-06 06:57:21 PST -------
(From update of attachment 172540 [details])
Attachment 172540 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14726908
------- Comment #6 From 2012-11-07 05:14:50 PST -------
Created an attachment (id=172763) [details]
DFG for MIPS.

Macro fix to make compilable on non-MIPS targets.
------- Comment #7 From 2012-11-08 12:04:15 PST -------
Created an attachment (id=173088) [details]
DFG for MIPS.

Rebased.
------- Comment #8 From 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 From 2012-11-14 10:16:48 PST -------
Created an attachment (id=174190) [details]
DFG for MIPS.
------- Comment #10 From 2012-11-14 10:25:09 PST -------
(From update of attachment 174190 [details])
Attachment 174190 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14844138
------- Comment #11 From 2012-11-14 10:35:02 PST -------
(From update of attachment 174190 [details])
Attachment 174190 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14844143
------- Comment #12 From 2012-11-14 10:35:42 PST -------
Created an attachment (id=174198) [details]
DFG for MIPS.
------- Comment #13 From 2012-11-14 10:38:38 PST -------
(From update of attachment 174198 [details])
Rebased on r134612
------- Comment #14 From 2012-12-18 03:27:59 PST -------
Created an attachment (id=179915) [details]
DFG for MIPS.
------- Comment #15 From 2012-12-18 03:30:11 PST -------
(From update of attachment 179915 [details])
Rebased on r138003.
------- Comment #16 From 2012-12-21 02:09:40 PST -------
Created an attachment (id=180494) [details]
DFG for MIPS.

replaceWithJump problem (described in Bug #103146) fix added.
------- Comment #17 From 2013-01-04 09:38:24 PST -------
Created an attachment (id=181319) [details]
DFG for MIPS.
------- Comment #18 From 2013-01-04 10:04:48 PST -------
(From update of attachment 181319 [details])
Rebased on r138804.
------- Comment #19 From 2013-01-05 23:25:38 PST -------
(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. :-/
------- Comment #20 From 2013-01-07 02:19:21 PST -------
(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.
------- Comment #21 From 2013-01-07 10:54:44 PST -------
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 181319 [details] [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 From 2013-01-07 11:06:36 PST -------
(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
>>>> +
>>> 
>>> 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 From 2013-01-07 11:07:43 PST -------
(From update of attachment 179915 [details])
I assume that this is an earlier version of the patch, so I'm clearing r?.
------- Comment #24 From 2013-01-08 08:10:14 PST -------
Created an attachment (id=181695) [details]
DFG for MIPS.

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

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

Rebased on r140318.
------- Comment #31 From 2013-01-29 04:38:13 PST -------
Created an attachment (id=185217) [details]
DFG for MIPS - arg. offset with inline

Rebased on r 41096.
------- Comment #32 From 2013-01-29 04:46:05 PST -------
Created an attachment (id=185219) [details]
DFG for MIPS - arg. offset with macro

Rebased on r141096.
------- Comment #33 From 2013-01-29 04:47:16 PST -------
(From update of attachment 185217 [details])
Rebased on r141096.
------- Comment #34 From 2013-02-11 11:07:10 PST -------
Created an attachment (id=187616) [details]
DFG for MIPS - arg. offset with inline

Rebased on r142489.
------- Comment #35 From 2013-02-18 07:04:04 PST -------
Created an attachment (id=188878) [details]
DFG for MIPS - arg. offset with inline

Rebased on r143211.
------- Comment #36 From 2013-02-18 07:12:42 PST -------
Created an attachment (id=188879) [details]
DFG for MIPS - arg. offset with macro

Rebased on r143211.
------- Comment #37 From 2013-02-18 11:27:04 PST -------
(From update of attachment 188879 [details])
Clearing flags on attachment: 188879

Committed r143247: <http://trac.webkit.org/changeset/143247>
------- Comment #38 From 2013-09-05 08:49:41 PST -------
(From update of attachment 188878 [details])
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.