Summary: | MIPS DFG implementation. | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Balazs Kilvady <kilvadyb> | ||||||||||||||||||||||||||||||||||||||||
Component: | JavaScriptCore | Assignee: | 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
Balazs Kilvady
2012-11-06 02:47:50 PST
Created attachment 172540 [details]
DFG for MIPS
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 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 on attachment 172540 [details] DFG for MIPS Attachment 172540 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14747377 Comment on attachment 172540 [details] DFG for MIPS Attachment 172540 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14726908 Created attachment 172763 [details]
DFG for MIPS.
Macro fix to make compilable on non-MIPS targets.
Created attachment 173088 [details]
DFG for MIPS.
Rebased.
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! Created attachment 174190 [details]
DFG for MIPS.
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 on attachment 174190 [details] DFG for MIPS. Attachment 174190 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14844143 Created attachment 174198 [details]
DFG for MIPS.
Comment on attachment 174198 [details] DFG for MIPS. Rebased on r134612 Created attachment 179915 [details]
DFG for MIPS.
Comment on attachment 179915 [details] DFG for MIPS. Rebased on r138003. Created attachment 180494 [details] DFG for MIPS. replaceWithJump problem (described in Bug #103146) fix added. Created attachment 181319 [details]
DFG for MIPS.
Comment on attachment 181319 [details] DFG for MIPS. Rebased on r138804. 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. :-/ (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. (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 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 on attachment 179915 [details]
DFG for MIPS.
I assume that this is an earlier version of the patch, so I'm clearing r?.
Created attachment 181695 [details]
DFG for MIPS.
With the requested poke abstraction which is implemented with a helper inline template function.
Created attachment 181698 [details]
DFG for MIPS #2.
With the requested poke abstraction which is implemented with an offset macro.
(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. Created attachment 183415 [details]
DFG for MIPS - arg. offset with macro
Created attachment 183416 [details]
DFG for MIPS - arg. offset with inline
Created attachment 183746 [details]
DFG for MIPS - arg. offset with inline
Created attachment 183750 [details] DFG for MIPS - arg. offset with macro Rebased on r140318. Created attachment 185217 [details]
DFG for MIPS - arg. offset with inline
Rebased on r 41096.
Created attachment 185219 [details] DFG for MIPS - arg. offset with macro Rebased on r141096. Comment on attachment 185217 [details] DFG for MIPS - arg. offset with inline Rebased on r141096. Created attachment 187616 [details] DFG for MIPS - arg. offset with inline Rebased on r142489. Created attachment 188878 [details] DFG for MIPS - arg. offset with inline Rebased on r143211. Created attachment 188879 [details] DFG for MIPS - arg. offset with macro Rebased on r143211. 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 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. |