X64: Implement RegExp natively.

X64: Implement RegExp natively.

[Lasse Reichstein, 2009-08-13 08:24:14]

Please review RegExp conversion.
I removed the unused "dispatch" functions in regexp-macro-assembler.h and
implementations, but otherwise the changes should be contained in the x64
directory and jsregexp.cc.

[William Hesse, 2009-08-13 09:28:08]

All files except the two new x64 ones reviewed.  Will send their review later.

http://codereview.chromium.org/165443/diff/42/1006
File src/jsregexp.cc (right):

http://codereview.chromium.org/165443/diff/42/1006#newcode469
Line 469: // If result is RETRY, the string have changed representation, and we
Grammar.  "string have"

http://codereview.chromium.org/165443/diff/42/1014
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/165443/diff/42/1014#newcode449
Line 449: void Assembler::arithmetic_op_16(byte opcode, Register dst, Register
src) {
These, and the other ones, should be reg, rm_reg.

http://codereview.chromium.org/165443/diff/42/1014#newcode814
Line 814: emit(0x3c);
Shouldn't imm8.value_ be emitted?

http://codereview.chromium.org/165443/diff/42/1015
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/165443/diff/42/1015#newcode288
Line 288: private:
This looks wrongly formatted.  Shouldn't the blank line come before "private:"

http://codereview.chromium.org/165443/diff/42/1015#newcode543
Line 543: arithmetic_op_32(0x02, src, dst);
I think you need 0x01 here, not 0x02.

http://codereview.chromium.org/165443/diff/42/1015#newcode588
Line 588: arithmetic_op(0x38, dst, src);
I think this is backwards.  0x38 is cmp r/m, reg.
The second argument of arithmetic op is reg_rm, I think.

Should you change all reg, reg operations to avoid the SIB byte for sp/r12, or
was it just important for addl?

http://codereview.chromium.org/165443/diff/42/1015#newcode600
Line 600: immediate_arithmetic_op_8(0x7, dst, src);
Should this test if dst is al, and call cmpb_al?
If so, why is cmpb_al publicly exposed?

http://codereview.chromium.org/165443/diff/42/1015#newcode1193
Line 1193: void arithmetic_op_32(byte opcode, Register dst, Register src);
All of these should say reg, rm_reg, not dst, src, to agree with their
parameters in the .cc file.  The two 64-bit ones should be together.

[William Hesse, 2009-08-13 11:12:22]

LGTM.

http://codereview.chromium.org/165443/diff/42/1016
File src/x64/regexp-macro-assembler-x64.cc (right):

http://codereview.chromium.org/165443/diff/42/1016#newcode68
Line 68: *    - stack_area_top        (High end of the memory area to use as
I don't like "top" here.  high_end, or backtrack_stack_base, or something else
is better.  Top means both high and top(low).

http://codereview.chromium.org/165443/diff/42/1016#newcode87
Line 87: * The first seven data must provided by the calling code, e.g., by
calling the
Grammar.

http://codereview.chromium.org/165443/diff/42/1016#newcode95
Line 95: * Entry code will ensure that the values are stored correctly if passed
in
This means that calls to this from generated code (not C), will depend on the
platform (Windows or Linux/MacOS).  I don't like the "e.g.".

http://codereview.chromium.org/165443/diff/42/1016#newcode227
Line 227: // TODO(lrn): Test multiple characters at a time by loading 4 or 8
bytes
Make this an issue, with a number and an owner.

http://codereview.chromium.org/165443/diff/42/1016#newcode329
Line 329: #ifdef WIN64
Should these definitions be a preprocessor macro, to be defined in
assembler-x64.h?  I mean the macro would expand to these definitions, not that
the actual names argument1,... would be macros.

[Lasse Reichstein, 2009-08-13 13:32:30]

Addressed comments

http://codereview.chromium.org/165443/diff/42/1014
File src/x64/assembler-x64.cc (right):

http://codereview.chromium.org/165443/diff/42/1014#newcode449
Line 449: void Assembler::arithmetic_op_16(byte opcode, Register dst, Register
src) {
Done

http://codereview.chromium.org/165443/diff/42/1014#newcode814
Line 814: emit(0x3c);
Most likely :)

http://codereview.chromium.org/165443/diff/42/1015
File src/x64/assembler-x64.h (right):

http://codereview.chromium.org/165443/diff/42/1015#newcode543
Line 543: arithmetic_op_32(0x02, src, dst);
Well spotted.

http://codereview.chromium.org/165443/diff/42/1015#newcode588
Line 588: arithmetic_op(0x38, dst, src);
I should do it for all of them, eventually. The addl one was just a trial to see
how much work it was. 
I'll leave a general change for another update.

http://codereview.chromium.org/165443/diff/42/1015#newcode600
Line 600: immediate_arithmetic_op_8(0x7, dst, src);
This one has an operand as destination, so it won't be al.
If there was a register version, it should probably check for al.

There is a tension here between making an optimizing assembler (picking the most
efficient opcode that does the same thing) and an exact assembler (does exactly
what it's told). The latter can be important if one is creating code that must
have a specific length or other known code qualities (i.e., something might
break if we change the length of an operation), whereas the former is obviously
easier to use to just write code to run.

http://codereview.chromium.org/165443/diff/42/1015#newcode1193
Line 1193: void arithmetic_op_32(byte opcode, Register dst, Register src);
Done.

http://codereview.chromium.org/165443/diff/42/1016
File src/x64/regexp-macro-assembler-x64.cc (right):

http://codereview.chromium.org/165443/diff/42/1016#newcode68
Line 68: *    - stack_area_top        (High end of the memory area to use as
Changed to "base".

http://codereview.chromium.org/165443/diff/42/1016#newcode95
Line 95: * Entry code will ensure that the values are stored correctly if passed
in
Removed final sentence and the "e.g.".
This is how you must call the code (and yes, calling convention depends on
platform and abi).

http://codereview.chromium.org/165443/diff/42/1016#newcode227
Line 227: // TODO(lrn): Test multiple characters at a time by loading 4 or 8
bytes
Done.

http://codereview.chromium.org/165443/diff/42/1016#newcode329
Line 329: #ifdef WIN64
They should probably just be removed, as they are the other places where we call
C++ code. It'll even make for tighter code on Windows.

[Erik Corry, 2009-08-13 13:54:19]

LGTM

http://codereview.chromium.org/165443/diff/42/1004
File src/ia32/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/165443/diff/42/1004#newcode1165
Line 1165: Address new_stack_top = RegExpStack::EnsureCapacity(size * 2);
Shouldn't new_stack_top be new_stack_base?

http://codereview.chromium.org/165443/diff/42/1006
File src/jsregexp.cc (right):

http://codereview.chromium.org/165443/diff/42/1006#newcode471
Line 471: } while (res == RegExpMacroAssemblerX64::RETRY);
I think you should move these enums up a level so they are the same for 32 and
64 bit.  Then you could get rid of this nasty big ifdef.  There's a lot of
duplicate code in the two branches.

http://codereview.chromium.org/165443/diff/42/1006#newcode488
Line 488: #endif  // V8_TARGET_ARCH_IA32
Wrong comment.

http://codereview.chromium.org/165443/diff/42/1006#newcode4505
Line 4505: RegExpMacroAssemblerX64::Mode mode;
Again, lets try to keep the amount of copied code to a minimum.

http://codereview.chromium.org/165443/diff/42/1013
File src/regexp-stack.h (right):

http://codereview.chromium.org/165443/diff/42/1013#newcode77
Line 77: static const uint32_t kMemoryTop = 0xffffffff;
Does this work on 64 bit?

http://codereview.chromium.org/165443/diff/42/1016
File src/x64/regexp-macro-assembler-x64.cc (right):

http://codereview.chromium.org/165443/diff/42/1016#newcode47
Line 47: *         Please notice that this is the byte offset, not the character
notice -> note

http://codereview.chromium.org/165443/diff/42/1016#newcode64
Line 64: * Each call to a C++ method should retain these registers.
Meaning of the words 'retain' and 'these' is unclear.

http://codereview.chromium.org/165443/diff/42/1016#newcode121
Line 121: // Unuse labels in case we throw away the assembler without calling
GetCode.
I wonder whether we ever do this any more.  Looks like a leftover from when we
still had the pcre-based code to fall back on.

http://codereview.chromium.org/165443/diff/42/1016#newcode579
Line 579: // No custom implementation (yet): w, W, s(UC16), S(UC16).
We should fix this.

http://codereview.chromium.org/165443/diff/42/1016#newcode605
Line 605: // MSVC passes arguments in rcx, rdx, r8, r9, with backing stack
slots.
This is presumable a property of the ABI and not the compiler.

http://codereview.chromium.org/165443/diff/42/1016#newcode794
Line 794: #ifndef WIN64
I wonder if we can reduce the number of ifdefs with some strategic predefined
macros.

http://codereview.chromium.org/165443/diff/42/1016#newcode935
Line 935: __ movq(register_location(register_index), Immediate(to));
Seems like movl is enough here.  Or is the smallest possible version
automatically emitted?

http://codereview.chromium.org/165443/diff/42/1016#newcode1308
Line 1308: // Notice: This updates flags, unlike normal Push.
Replace notice with note throughout.

http://codereview.chromium.org/165443/diff/42/1016#newcode1324
Line 1324: // The position succeeds a relative label offset from position.
Somehow the grammar in this comment is strange.

http://codereview.chromium.org/165443/diff/42/1017
File src/x64/regexp-macro-assembler-x64.h (right):

http://codereview.chromium.org/165443/diff/42/1017#newcode130
Line 130: // Offsets from ebp of function parameters and stored registers.
rbp?

http://codereview.chromium.org/165443/diff/42/1017#newcode136
Line 136: #ifdef WIN64
I'd rather have two ifdefs than this huge one.  A comment as to why there is a
difference between WIN64 and non-WIN64 would be nice too.

[Lasse Reichstein, 2009-08-14 08:08:14]

Is the refactoring satisfactory?

http://codereview.chromium.org/165443/diff/42/1004
File src/ia32/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/165443/diff/42/1004#newcode1165
Line 1165: Address new_stack_top = RegExpStack::EnsureCapacity(size * 2);
That would be consistent. Fixed.

http://codereview.chromium.org/165443/diff/42/1006
File src/jsregexp.cc (right):

http://codereview.chromium.org/165443/diff/42/1006#newcode471
Line 471: } while (res == RegExpMacroAssemblerX64::RETRY);
This definitly needs refactoring. Thanks for pushing it :)
I'll upload a new version when I have it prettified.

http://codereview.chromium.org/165443/diff/42/1006#newcode4505
Line 4505: RegExpMacroAssemblerX64::Mode mode;
Done.

http://codereview.chromium.org/165443/diff/42/1013
File src/regexp-stack.h (right):

http://codereview.chromium.org/165443/diff/42/1013#newcode77
Line 77: static const uint32_t kMemoryTop = 0xffffffff;
Probably not. It won't get hit by the current code, since we are careful never
to use a RegExpStack that hasn't been allocated. I'll change it to uintptr_t -1.

[Erik Corry, 2009-08-14 08:20:55]

LGTM

http://codereview.chromium.org/165443/diff/1026/1037
File src/regexp-macro-assembler.cc (right):

http://codereview.chromium.org/165443/diff/1026/1037#newcode45
Line 45: NativeRegExpMacroAssembler::NativeRegExpMacroAssembler() {
Can we ensure this doesn't get compiled on ARM.