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

Balazs Kilvady
Reported 2012-11-06 02:47:50 PST
DFG implementation for MIPS.
Attachments
DFG for MIPS (44.79 KB, patch)
2012-11-06 03:11 PST, Balazs Kilvady 		webkit-ews: commit-queue-
DetailsFormatted DiffDiff
DFG for MIPS. (44.76 KB, patch)
2012-11-07 05:14 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS. (44.83 KB, patch)
2012-11-08 12:04 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS. (46.88 KB, patch)
2012-11-14 10:16 PST, Balazs Kilvady 		webkit-ews: commit-queue-
DetailsFormatted DiffDiff
DFG for MIPS. (46.91 KB, patch)
2012-11-14 10:35 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS. (50.05 KB, patch)
2012-12-18 03:27 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS. (50.48 KB, patch)
2012-12-21 02:09 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS. (50.62 KB, patch)
2013-01-04 09:38 PST, Balazs Kilvady 		fpizlo: review-
fpizlo: commit-queue-
DetailsFormatted DiffDiff
DFG for MIPS. (51.23 KB, patch)
2013-01-08 08:10 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS #2. (51.44 KB, patch)
2013-01-08 08:19 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with macro (51.46 KB, patch)
2013-01-18 02:46 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with inline (51.25 KB, patch)
2013-01-18 02:55 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with inline (51.19 KB, patch)
2013-01-21 03:24 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with macro (51.40 KB, patch)
2013-01-21 03:36 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with inline (51.24 KB, patch)
2013-01-29 04:38 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with macro (51.46 KB, patch)
2013-01-29 04:46 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with inline (51.24 KB, patch)
2013-02-11 11:07 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with inline (51.24 KB, patch)
2013-02-18 07:04 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
DFG for MIPS - arg. offset with macro (51.45 KB, patch)
2013-02-18 07:12 PST, Balazs Kilvady 		no flags
DetailsFormatted DiffDiff
Show Obsolete (17) View All Add attachment proposed patch, testcase, etc.
Balazs Kilvady
Comment 1 2012-11-06 03:11:23 PST
Created attachment 172540 [details] DFG for MIPS
Early Warning System Bot
Comment 2 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
Early Warning System Bot
Comment 3 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
EFL EWS Bot
Comment 4 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
Build Bot
Comment 5 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
Balazs Kilvady
Comment 6 2012-11-07 05:14:50 PST
Created attachment 172763 [details] DFG for MIPS. Macro fix to make compilable on non-MIPS targets.
Balazs Kilvady
Comment 7 2012-11-08 12:04:15 PST
Created attachment 173088 [details] DFG for MIPS. Rebased.
Chao-ying Fu
Comment 8 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!
Balazs Kilvady
Comment 9 2012-11-14 10:16:48 PST
Created attachment 174190 [details] DFG for MIPS.
Early Warning System Bot
Comment 10 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
EFL EWS Bot
Comment 11 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
Balazs Kilvady
Comment 12 2012-11-14 10:35:42 PST
Created attachment 174198 [details] DFG for MIPS.
Balazs Kilvady
Comment 13 2012-11-14 10:38:38 PST
Comment on attachment 174198 [details] DFG for MIPS. Rebased on r134612
Balazs Kilvady
Comment 14 2012-12-18 03:27:59 PST
Created attachment 179915 [details] DFG for MIPS.
Balazs Kilvady
Comment 15 2012-12-18 03:30:11 PST
Comment on attachment 179915 [details] DFG for MIPS. Rebased on r138003.
Balazs Kilvady
Comment 16 2012-12-21 02:09:40 PST
Created attachment 180494 [details] DFG for MIPS. replaceWithJump problem (described in Bug #103146) fix added.
Balazs Kilvady
Comment 17 2013-01-04 09:38:24 PST
Created attachment 181319 [details] DFG for MIPS.
Balazs Kilvady
Comment 18 2013-01-04 10:04:48 PST
Comment on attachment 181319 [details] DFG for MIPS. Rebased on r138804.
Filip Pizlo
Comment 19 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. :-/
Balazs Kilvady
Comment 20 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.
Filip Pizlo
Comment 21 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.
Filip Pizlo
Comment 22 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.
Filip Pizlo
Comment 23 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?.
Balazs Kilvady
Comment 24 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.
Balazs Kilvady
Comment 25 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.
Balazs Kilvady
Comment 26 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.
Balazs Kilvady
Comment 27 2013-01-18 02:46:40 PST
Created attachment 183415 [details] DFG for MIPS - arg. offset with macro
Balazs Kilvady
Comment 28 2013-01-18 02:55:35 PST
Created attachment 183416 [details] DFG for MIPS - arg. offset with inline
Balazs Kilvady
Comment 29 2013-01-21 03:24:00 PST
Created attachment 183746 [details] DFG for MIPS - arg. offset with inline
Balazs Kilvady
Comment 30 2013-01-21 03:36:21 PST
Created attachment 183750 [details] DFG for MIPS - arg. offset with macro Rebased on r140318.
Balazs Kilvady
Comment 31 2013-01-29 04:38:13 PST
Created attachment 185217 [details] DFG for MIPS - arg. offset with inline Rebased on r 41096.
Balazs Kilvady
Comment 32 2013-01-29 04:46:05 PST
Created attachment 185219 [details] DFG for MIPS - arg. offset with macro Rebased on r141096.
Balazs Kilvady
Comment 33 2013-01-29 04:47:16 PST
Comment on attachment 185217 [details] DFG for MIPS - arg. offset with inline Rebased on r141096.
Balazs Kilvady
Comment 34 2013-02-11 11:07:10 PST
Created attachment 187616 [details] DFG for MIPS - arg. offset with inline Rebased on r142489.
Balazs Kilvady
Comment 35 2013-02-18 07:04:04 PST
Created attachment 188878 [details] DFG for MIPS - arg. offset with inline Rebased on r143211.
Balazs Kilvady
Comment 36 2013-02-18 07:12:42 PST
Created attachment 188879 [details] DFG for MIPS - arg. offset with macro Rebased on r143211.
WebKit Review Bot
Comment 37 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>
Zan Dobersek
Comment 38 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.
Note You need to log in before you can comment on or make changes to this bug.