MIPS: crankshaft implementation

MIPS: crankshaft implementation


BUG=
TEST=

Committed: http://code.google.com/p/v8/source/detail?r=9828

[Paul Lind, 2011-09-17 02:39:52]

Hi guys - sorry to drop a bomb on you first thing Monday morning, but here at
long last is our crankshaft port, in one big commit. 

Søren said he he checked with you, and this is the preferred way to submit it.

I have based this on top of: http://codereview.chromium.org/7891033/ which has
not been landed yet, because there was a merge problem there with
code-stubs-mips.cc. Hopefully that will get landed before you get too far into
this. 

To pass debug tests you'll also need http://codereview.chromium.org/7932010/
(and this is pending also: http://codereview.chromium.org/7934003/)

With those, It builds fine for me and runs with only one mozilla fail, which
also failed on arm: js1_5/extensions/regress-355497.js

Looking forward to your reviews, I will turn them around as fast as I can.

thanks,

paul

[Paul Lind, 2011-10-17 05:13:52]

Hi guys - 

Here is the resubmission of our crankshaft code, with new-gc, and all recent
ports, current to r9640.

thanks,

paul

[fschneider, 2011-10-18 14:27:58]

First round of feedback. I looked at the files up to including
lithium-codegen.h.

http://codereview.chromium.org/7934002/diff/9001/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/7934002/diff/9001/src/flag-definitions.h#newco...
src/flag-definitions.h:322: DEFINE_bool(check_icache, false, "Check icache
flushes in ARM and MIPS simulator")
Long line.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc
File src/mips/deoptimizer-mips.cc (right):

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:116: 
Remove extra new line.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:176: // ----- pc_after points here
With new GC I think you also need a write barrier here using RecordWriteIntoCode
(see ia32 and other platforms).

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:194: check_code->entry());
With new GC also insert call to write barrier here.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:589: __ sw(ToRegister(i), MemOperand(sp, 4 * i));
Maybe use kPointerSize instead of 4.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:705: // __ add(r1, r0, Operand(r1, LSL, 2));
Remove commented code. Looks as if this left over from the ARM file?

http://codereview.chromium.org/7934002/diff/9001/src/mips/lithium-codegen-mips.h
File src/mips/lithium-codegen-mips.h (right):

http://codereview.chromium.org/7934002/diff/9001/src/mips/lithium-codegen-mip...
src/mips/lithium-codegen-mips.h:210: // to be in edi.
Wrong register in comment: Function expected in a1.

[Paul Lind, 2011-10-19 02:18:27]

Thanks for the review. Updated per your comments, and rebased to today's
changes.

The changes for r9641 (Fix evaluation order of GT and LTE operators.) have been
ported and submitted separately  in http://codereview.chromium.org/8348028/,
which will need to be landed before this code will pass all tests. Note that the
crankshaft portions of that change are now included in this issue.

http://codereview.chromium.org/7934002/diff/9001/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/7934002/diff/9001/src/flag-definitions.h#newco...
src/flag-definitions.h:322: DEFINE_bool(check_icache, false, "Check icache
flushes in ARM and MIPS simulator")
On 2011/10/18 14:27:58, fschneider wrote:
> Long line.

Done.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc
File src/mips/deoptimizer-mips.cc (right):

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:116: 
On 2011/10/18 14:27:58, fschneider wrote:
> Remove extra new line.

Done.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:176: // ----- pc_after points here
On 2011/10/18 14:27:58, fschneider wrote:
> With new GC I think you also need a write barrier here using
RecordWriteIntoCode
> (see ia32 and other platforms).

Oops, we missed this. Thanks. Added it, and ported your refactoring from r9687.

Also, I added an ASSERT that seemed like a good idea, and updated some comments.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:194: check_code->entry());
On 2011/10/18 14:27:58, fschneider wrote:
> With new GC also insert call to write barrier here.

Updated, as above.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:589: __ sw(ToRegister(i), MemOperand(sp, 4 * i));
On 2011/10/18 14:27:58, fschneider wrote:
> Maybe use kPointerSize instead of 4.

Done.

http://codereview.chromium.org/7934002/diff/9001/src/mips/deoptimizer-mips.cc...
src/mips/deoptimizer-mips.cc:705: // __ add(r1, r0, Operand(r1, LSL, 2));
On 2011/10/18 14:27:58, fschneider wrote:
> Remove commented code. Looks as if this left over from the ARM file?

Yep. A porting artifact, now removed.

http://codereview.chromium.org/7934002/diff/9001/src/mips/lithium-codegen-mips.h
File src/mips/lithium-codegen-mips.h (right):

http://codereview.chromium.org/7934002/diff/9001/src/mips/lithium-codegen-mip...
src/mips/lithium-codegen-mips.h:210: // to be in edi.
On 2011/10/18 14:27:58, fschneider wrote:
> Wrong register in comment: Function expected in a1.

Done.

[Kevin Millikin (Chromium), 2011-10-19 07:43:21]

I looked at all but the lithium-mips.{h,cc} and lithium-codegen-mips.{h,cc}
files.  The rest seems OK to me.

http://codereview.chromium.org/7934002/diff/15001/src/SConscript
File src/SConscript (right):

http://codereview.chromium.org/7934002/diff/15001/src/SConscript#newcode175
src/SConscript:175: mips/lithium-mips.cc
It's not really very important, but I guess we alphabetize these ('M-x
sort-lines' in emacs), codegen, gap-resolver, mips.

http://codereview.chromium.org/7934002/diff/15001/src/mips/lithium-gap-resolv...
File src/mips/lithium-gap-resolver-mips.cc (right):

http://codereview.chromium.org/7934002/diff/15001/src/mips/lithium-gap-resolv...
src/mips/lithium-gap-resolver-mips.cc:40: : cgen_(owner), moves_(32),
root_index_(0), in_cycle_(false),
We normally put one initializer per line if they don't all fit on the same line.

[Paul Lind, 2011-10-20 04:34:08]

Kevin, I updated for the comments you left. I was not able to rebase this on
today's activity yet, as I have a couple bugs in the ported commits. I'll get it
in as soon as I can, but will likely be after your end of day tomorrow.

thanks,

paul

http://codereview.chromium.org/7934002/diff/15001/src/SConscript
File src/SConscript (right):

http://codereview.chromium.org/7934002/diff/15001/src/SConscript#newcode175
src/SConscript:175: mips/lithium-mips.cc
On 2011/10/19 07:43:21, Kevin Millikin wrote:
> It's not really very important, but I guess we alphabetize these ('M-x
> sort-lines' in emacs), codegen, gap-resolver, mips.

Done. I suppose maybe we should then move the entire arch:mips section below
arch:ia32 for same reason, but I did not do that here. Let me know if you want
that also.

http://codereview.chromium.org/7934002/diff/15001/src/mips/lithium-gap-resolv...
File src/mips/lithium-gap-resolver-mips.cc (right):

http://codereview.chromium.org/7934002/diff/15001/src/mips/lithium-gap-resolv...
src/mips/lithium-gap-resolver-mips.cc:40: : cgen_(owner), moves_(32),
root_index_(0), in_cycle_(false),
On 2011/10/19 07:43:21, Kevin Millikin wrote:
> We normally put one initializer per line if they don't all fit on the same
line.

Done.

[Paul Lind, 2011-10-21 05:38:38]

The previous upload covered Kevin's most recent comments. This rebases on r9730.


This includes the crankshaft portions of these following changes:

9676     Make the GC aware of JSReceiver pointers in LookupResults.
9687     Add write barrier helper for code patching and refactor stack check
patching.
9698     Support array literals with FAST_DOUBLE_ELEMENTS ElementsKind.
9699     Allow inlining of named function expressions containing ThisFunction
reference.
9702     Introduce HTransitionElementsKind instruction.
9718     Speed up comparison with a constant. 
9722     Shave 39% from snapshot size.

I've submitted 4 other change-sets today for the other parts of the mips port,
and if/when those are landed, we should be passing all tests here.

    http://codereview.chromium.org/8366031
    http://codereview.chromium.org/8362024
    http://codereview.chromium.org/8363025
    http://codereview.chromium.org/8365026

[Paul Lind, 2011-10-24 04:36:50]

Rebased to r 9745, and made a small improvement to DoTransitionElementsKind.

This will require http://codereview.chromium.org/8368021/ to be landed, which
fixes the build for mip, then this will run fine.

[Paul Lind, 2011-10-26 02:48:32]

Hi guys, this was rebased on rr9779, but I uploaded several patches today which
need to be landed before this will work.

http://codereview.chromium.org/8390030
http://codereview.chromium.org/8395030
http://codereview.chromium.org/8390032
http://codereview.chromium.org/8396029
http://codereview.chromium.org/8390033
http://codereview.chromium.org/8394028
http://codereview.chromium.org/8390035


Also, these 2 were previously submitted:
http://codereview.chromium.org/8378001
http://codereview.chromium.org/8368021
thanks,

paul

[Paul Lind, 2011-10-27 04:10:24]

Hi guys, thanks to you landing all my patches from yesterday, there is only one
patch that needs to be landed today in order to make the mips version of
crankshaft buildable/testable:

http://codereview.chromium.org/8404016/

[Paul Lind, 2011-10-28 00:48:07]

Rebased on today's changes (r9823). It will build and run directly on the head.
No other cl's are needed today.

thanks,

paul

[Kevin Millikin (Chromium), 2011-10-28 08:15:48]

Looks good to us, we will land it.