Port direct call from JavaScript to native RegExp to x64...

Port direct call from JavaScript to native RegExp to x64

Code tested on both Linux and Windows.

Added a bit more abstraction to calling a C function from generated code.

Minor tweaks to the ia32 version.

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

[Lasse Reichstein, 2010-01-28 12:39:46]

LGTM with a few corrections.

http://codereview.chromium.org/548179/diff/18/3005
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/548179/diff/18/3005#newcode6571
src/x64/codegen-x64.cc:6571: if (!FLAG_regexp_entry_native) {
You could check up-front whether we are compiled with native regexps.
I.e.,
#ifndef V8_NATIVE_REGEXP
   // call runtime always (which calls the bytecode interpreter)
#endif
The code still works, since the generated code will be a ByteArray, not a Code
object, so we go to runtime when that is checked, but only after a lot of wasted
checks.

http://codereview.chromium.org/548179/diff/18/3005#newcode6605
src/x64/codegen-x64.cc:6605: __ j(is_smi, &runtime);
Use JumpIfSmi(rax, &runtime);
Generally, the sequence of 
 Condition c = CheckSomething()
 j(c, somewhere)
should have a JumpIfSomething(somewhere) shorthand (and probably also a
JumpIfNotSomething(somewhere)). It's MUCH more readable. If it isn't there, do
add it.

Using the Check macros directly should be reserved for code that expects a
condition as part of a more complicated dispatch (e.g., virtual frame
functions).

http://codereview.chromium.org/548179/diff/18/3005#newcode6638
src/x64/codegen-x64.cc:6638: is_smi = masm->CheckSmi(rax);
JumpIfSmi

http://codereview.chromium.org/548179/diff/18/3005#newcode6651
src/x64/codegen-x64.cc:6651: __ j(NegateCondition(is_smi), &runtime);
You are not checking that it's positive.
You can use JumpIfNotPositiveSmi

http://codereview.chromium.org/548179/diff/18/3005#newcode6653
src/x64/codegen-x64.cc:6653: __ SmiCompare(rax, rbx);
rbx is not a smi.
You should un-smi-tag rax before comparing.
Or, perhaps, make a LoadSmiToInteger32 macro that loads a smi and converts it to
an int (which can be done efficiently by loading the top half of the smi).

If you untag and compare as int32's, you can do unsigned comparison to fail if
the value is negative.

http://codereview.chromium.org/548179/diff/18/3005#newcode6670
src/x64/codegen-x64.cc:6670: __ movq(rax, FieldOperand(rbx,
FixedArray::kLengthOffset));
The FixedArray::length is an int, use movl to load it.

Mention that a FixedArray can't have a length so large that
length+kLastMatchOverhead overflows.

http://codereview.chromium.org/548179/diff/18/3005#newcode6694
src/x64/codegen-x64.cc:6694: // a sequential string.
Are we sure that sequential string haven't been turned into an external string?
I.e., I think it could be external.
(This seems to be assumed in ia32 too).

http://codereview.chromium.org/548179/diff/18/3005#newcode6705
src/x64/codegen-x64.cc:6705: __ andl(rbx,
Immediate(kStringRepresentationEncodingMask));
Consider adding andb to the assembler. It should be fairly simple (I'm guessing
   void andb(Register dst, Immediate src) {
     immediate_arithmetic_op_8(0x4, dst, src);
   }
should be sufficient, but hasn't been checked).

http://codereview.chromium.org/548179/diff/18/3005#newcode6730
src/x64/codegen-x64.cc:6730: // Check that the irregexp code has been generated
for If it has, the field
Missing words before "If".

http://codereview.chromium.org/548179/diff/18/3005#newcode6736
src/x64/codegen-x64.cc:6736: // rdi: encoding of subject string (1 if ascii 0 if
two_byte);
Comma before "0".

http://codereview.chromium.org/548179/diff/18/3005#newcode6756
src/x64/codegen-x64.cc:6756: __
ArgumentStackSlotsForCFunctionCall(kRegExpExecuteArguments);
use masm-> (or masm_-> or masm()->, whatever __ abbreviates) instead of __. When
computing code coverage, the __ macro can't return values.

http://codereview.chromium.org/548179/diff/18/3005#newcode6841
src/x64/codegen-x64.cc:6841: __ j(equal, &runtime);
We are effectively rerunning the regexp just to get an exception. But we know
that the exception is an out-of-regexp-stack exception (the only kind that is
generated inside the RegExp code and doesn't set the pending exception).
Could we have a runtime function that only raised that exception?

http://codereview.chromium.org/548179/diff/18/3005#newcode6853
src/x64/codegen-x64.cc:6853: __ SmiToInteger64(rdx, rdx);
You used SmiTimesPowerOfTwoToInteger64 above.

http://codereview.chromium.org/548179/diff/18/3005#newcode6892
src/x64/codegen-x64.cc:6892: __ movq(rdi, Operand(rcx, rdx, times_int_size, 0));
movl?

http://codereview.chromium.org/548179/diff/18/3005#newcode6896
src/x64/codegen-x64.cc:6896: // Carry flag set by smi convertion above.
Carry flag -> Negative flag.
Add it to the documentation of Integer32ToSmi that its assumed to set N and Z
flags depending on the outcome (so someone reimplementing it won't break your
code).

http://codereview.chromium.org/548179/diff/18/3006
File src/x64/regexp-macro-assembler-x64.cc (right):

http://codereview.chromium.org/548179/diff/18/3006#newcode332
src/x64/regexp-macro-assembler-x64.cc:332: // Caller save on Win64
"Callee save" is correct (but confusing, since we are not on Win64). So change
to "Caller save (but not on Win64)".

http://codereview.chromium.org/548179/diff/4007/5005
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/548179/diff/4007/5005#newcode2494
src/x64/macro-assembler-x64.cc:2494: // Make stack end at alignment and allocate
space for argument slots on stack
-> "arguments slots and old rsp on stack"

[Søren Thygesen Gjesse, 2010-01-28 14:48:21]

Thank you for the through review, that caught some nasty bugs. 

Please take another look.

http://codereview.chromium.org/548179/diff/18/3005
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/548179/diff/18/3005#newcode6571
src/x64/codegen-x64.cc:6571: if (!FLAG_regexp_entry_native) {
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> You could check up-front whether we are compiled with native regexps.
> I.e.,
> #ifndef V8_NATIVE_REGEXP
>    // call runtime always (which calls the bytecode interpreter)
> #endif
> The code still works, since the generated code will be a ByteArray, not a Code
> object, so we go to runtime when that is checked, but only after a lot of
wasted
> checks.

Good point. Added in ia32 as well. Are we ever testing a configuration without
native RegExp?

http://codereview.chromium.org/548179/diff/18/3005#newcode6605
src/x64/codegen-x64.cc:6605: __ j(is_smi, &runtime);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> Use JumpIfSmi(rax, &runtime);
> Generally, the sequence of 
>  Condition c = CheckSomething()
>  j(c, somewhere)
> should have a JumpIfSomething(somewhere) shorthand (and probably also a
> JumpIfNotSomething(somewhere)). It's MUCH more readable. If it isn't there, do
> add it.
> 
> Using the Check macros directly should be reserved for code that expects a
> condition as part of a more complicated dispatch (e.g., virtual frame
> functions).

Done.

http://codereview.chromium.org/548179/diff/18/3005#newcode6638
src/x64/codegen-x64.cc:6638: is_smi = masm->CheckSmi(rax);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> JumpIfSmi

Done.

http://codereview.chromium.org/548179/diff/18/3005#newcode6651
src/x64/codegen-x64.cc:6651: __ j(NegateCondition(is_smi), &runtime);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> You are not checking that it's positive.
> You can use JumpIfNotPositiveSmi

Good catch. Removed this check - see below.

http://codereview.chromium.org/548179/diff/18/3005#newcode6653
src/x64/codegen-x64.cc:6653: __ SmiCompare(rax, rbx);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> rbx is not a smi.
> You should un-smi-tag rax before comparing.
> Or, perhaps, make a LoadSmiToInteger32 macro that loads a smi and converts it
to
> an int (which can be done efficiently by loading the top half of the smi).
> 
> If you untag and compare as int32's, you can do unsigned comparison to fail if
> the value is negative.

Good catch. Now using SmiToInterger32 and unsigned compare which also fails on
negative. Changed like this on ia32 as well.

http://codereview.chromium.org/548179/diff/18/3005#newcode6670
src/x64/codegen-x64.cc:6670: __ movq(rax, FieldOperand(rbx,
FixedArray::kLengthOffset));
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> The FixedArray::length is an int, use movl to load it.
> 
> Mention that a FixedArray can't have a length so large that
> length+kLastMatchOverhead overflows.

Good catch. Added

ASSERT(FixedArray::kMaxLength < kMaxInt - FixedArray::kLengthOffset);

as well.

http://codereview.chromium.org/548179/diff/18/3005#newcode6694
src/x64/codegen-x64.cc:6694: // a sequential string.
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> Are we sure that sequential string haven't been turned into an external
string?
> I.e., I think it could be external.
> (This seems to be assumed in ia32 too).

Added check to ensure the first part is sequential. This has also been added in
the ia32 version.

http://codereview.chromium.org/548179/diff/18/3005#newcode6705
src/x64/codegen-x64.cc:6705: __ andl(rbx,
Immediate(kStringRepresentationEncodingMask));
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> Consider adding andb to the assembler. It should be fairly simple (I'm
guessing
>    void andb(Register dst, Immediate src) {
>      immediate_arithmetic_op_8(0x4, dst, src);
>    }
> should be sufficient, but hasn't been checked).

Done.

http://codereview.chromium.org/548179/diff/18/3005#newcode6730
src/x64/codegen-x64.cc:6730: // Check that the irregexp code has been generated
for If it has, the field
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> Missing words before "If".

Done.

http://codereview.chromium.org/548179/diff/18/3005#newcode6736
src/x64/codegen-x64.cc:6736: // rdi: encoding of subject string (1 if ascii 0 if
two_byte);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> Comma before "0".

Done.

http://codereview.chromium.org/548179/diff/18/3005#newcode6756
src/x64/codegen-x64.cc:6756: __
ArgumentStackSlotsForCFunctionCall(kRegExpExecuteArguments);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> use masm-> (or masm_-> or masm()->, whatever __ abbreviates) instead of __.
When
> computing code coverage, the __ macro can't return values.

Done.

http://codereview.chromium.org/548179/diff/18/3005#newcode6841
src/x64/codegen-x64.cc:6841: __ j(equal, &runtime);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> We are effectively rerunning the regexp just to get an exception. But we know
> that the exception is an out-of-regexp-stack exception (the only kind that is
> generated inside the RegExp code and doesn't set the pending exception).
> Could we have a runtime function that only raised that exception?

I think we could. Opened a bug and added a TODO(592).

http://codereview.chromium.org/548179/diff/18/3005#newcode6853
src/x64/codegen-x64.cc:6853: __ SmiToInteger64(rdx, rdx);
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> You used SmiTimesPowerOfTwoToInteger64 above.

Used the same code as above.

http://codereview.chromium.org/548179/diff/18/3005#newcode6892
src/x64/codegen-x64.cc:6892: __ movq(rdi, Operand(rcx, rdx, times_int_size, 0));
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> movl?

Done.

http://codereview.chromium.org/548179/diff/18/3005#newcode6896
src/x64/codegen-x64.cc:6896: // Carry flag set by smi convertion above.
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> Carry flag -> Negative flag.
> Add it to the documentation of Integer32ToSmi that its assumed to set N and Z
> flags depending on the outcome (so someone reimplementing it won't break your
> code).

Done.

http://codereview.chromium.org/548179/diff/18/3006
File src/x64/regexp-macro-assembler-x64.cc (right):

http://codereview.chromium.org/548179/diff/18/3006#newcode332
src/x64/regexp-macro-assembler-x64.cc:332: // Caller save on Win64
On 2010/01/28 12:39:46, Lasse Reichstein wrote:
> "Callee save" is correct (but confusing, since we are not on Win64). So change
> to "Caller save (but not on Win64)".
Sorry i got the double negation wrong. The comment is now more explicit.

http://codereview.chromium.org/548179/diff/4007/5005
File src/x64/macro-assembler-x64.cc (right):

http://codereview.chromium.org/548179/diff/4007/5005#newcode2494
src/x64/macro-assembler-x64.cc:2494: // Make stack end at alignment and allocate
space for argument slots on stack
On 2010/01/28 12:39:47, Lasse Reichstein wrote:
> -> "arguments slots and old rsp on stack"

Done.

[Lasse Reichstein, 2010-01-29 08:09:08]

LGTM

http://codereview.chromium.org/548179/diff/6001/7001
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/548179/diff/6001/7001#newcode8405
src/ia32/codegen-ia32.cc:8405: #endif
Perhaps use #else to make all the following code go away if regexp aren't
native.

http://codereview.chromium.org/548179/diff/6001/7005
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/548179/diff/6001/7005#newcode6654
src/x64/codegen-x64.cc:6654: __ j(greater, &runtime);
greater -> above

http://codereview.chromium.org/548179/diff/6001/7005#newcode6709
src/x64/codegen-x64.cc:6709: __ andb(rbx,
Immediate(kStringRepresentationEncodingMask));
Do you need the representation bits any more? You know that it's sequential.
I.e., you can just and with EncodingMask and use the Z flag directly to branch,
without a compare in between.

[Søren Thygesen Gjesse, 2010-01-29 13:06:53]

http://codereview.chromium.org/548179/diff/6001/7001
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/548179/diff/6001/7001#newcode8405
src/ia32/codegen-ia32.cc:8405: #endif
On 2010/01/29 08:09:08, Lasse Reichstein wrote:
> Perhaps use #else to make all the following code go away if regexp aren't
> native.

Done.

http://codereview.chromium.org/548179/diff/6001/7005
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/548179/diff/6001/7005#newcode6654
src/x64/codegen-x64.cc:6654: __ j(greater, &runtime);
On 2010/01/29 08:09:08, Lasse Reichstein wrote:
> greater -> above

Thanks, fixed in ia32 version as well.

http://codereview.chromium.org/548179/diff/6001/7005#newcode6709
src/x64/codegen-x64.cc:6709: __ andb(rbx,
Immediate(kStringRepresentationEncodingMask));
On 2010/01/29 08:09:08, Lasse Reichstein wrote:
> Do you need the representation bits any more? You know that it's sequential.
> I.e., you can just and with EncodingMask and use the Z flag directly to
branch,
> without a compare in between.

The reason is that the jump to label seq_string from above has both encoding and
representation in rbx.