ARM native regexps.

ARM native regexps.

[Lasse Reichstein, 2009-08-27 12:14:05]

Pretty large review.

[Erik Corry, 2009-08-27 14:43:05]

Here are the comments so far

http://codereview.chromium.org/173567/diff/1/4
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/173567/diff/1/4#newcode342
Line 342: return pos + 8 + imm26;
This isn't your code, but I wonder what this '8' really means.

http://codereview.chromium.org/173567/diff/1/4#newcode355
Line 355: int imm26 = target_pos - pos - 8;
And this one.

http://codereview.chromium.org/173567/diff/1/4#newcode383
Line 383: PrintF("value");
Seems like this should print what the value is and a newline.

http://codereview.chromium.org/173567/diff/1/4#newcode688
Line 688: return target_pos - pc_offset() - 8;
And this one!

http://codereview.chromium.org/173567/diff/1/5
File src/arm/assembler-arm.h (right):

http://codereview.chromium.org/173567/diff/1/5#newcode424
Line 424: // Puts a labels target address at the given position.
labels -> label's

http://codereview.chromium.org/173567/diff/1/5#newcode425
Line 425: // The high 8 bits are set to zero.
I wonder why?

http://codereview.chromium.org/173567/diff/1/5#newcode426
Line 426: void label_at_put(Label* L, int at_offset);
Gotta love that smalltalk syntax, but the index should come first for the full
beard-wearing barefoot experience!

http://codereview.chromium.org/173567/diff/1/7
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/173567/diff/1/7#newcode52
Line 52: #include "disasm.h"
We don't normally put includes in includes if the include itself doesn't need
it.

http://codereview.chromium.org/173567/diff/1/9
File src/arm/regexp-macro-assembler-arm.cc (right):

http://codereview.chromium.org/173567/diff/1/9#newcode81
Line 81: * character of the string). The remaining registers starts out as
garbage.
starts -> start

http://codereview.chromium.org/173567/diff/1/9#newcode85
Line 85: * int (*match)(String* input_string,
There should be a typedef for this in the .h file.

http://codereview.chromium.org/173567/diff/1/9#newcode140
Line 140: void RegExpMacroAssemblerARM::AdvanceRegister(int reg, int by) {
We should consider putting 'register 0' in a real register.

http://codereview.chromium.org/173567/diff/1/9#newcode155
Line 155: __ add(pc, r0, Operand(masm_->CodeObject()));
I like the alternative better, because the add above turns into a pc-relative
load into the scratch register, followed by the add.

http://codereview.chromium.org/173567/diff/1/9#newcode182
Line 182: __ ldrb(r0, MemOperand(frame_pointer(), kAtStart));
Don't use ldrb if you don't have to - costs a cycle.

http://codereview.chromium.org/173567/diff/1/9#newcode187
Line 187: __ add(r0, end_of_input_address(), Operand(current_input_offset()));
Reorder here to put the add in the delay slot of the load.

http://codereview.chromium.org/173567/diff/1/9#newcode201
Line 201: __ add(r0, end_of_input_address(), Operand(current_input_offset()));
And here.

http://codereview.chromium.org/173567/diff/1/9#newcode238
Line 238: __ cmp(r1, Operand(str[i]));
For 16 bit strings this will generate constant pool entries like animal!  How
about loading the high byte into a different register then for each iteration
you update the high byte if necessary (rarely) and or-in the low byte.  Or we
just have the string as a literal in the constant pool somehow.

http://codereview.chromium.org/173567/diff/1/9#newcode308
Line 308: // Restore original values before failing.
Misplaced comment?

http://codereview.chromium.org/173567/diff/1/9#newcode362
Line 362: // Succeed on empty capture (including no capture)
Missing full stop.

http://codereview.chromium.org/173567/diff/1/9#newcode546
Line 546: __ mov(r0, Operand(0));
Can't we just use Operand(FAILURE)?

http://codereview.chromium.org/173567/diff/1/9#newcode615
Line 615: // page (a problem on Windows).
Comment seems malplaced.

http://codereview.chromium.org/173567/diff/1/9#newcode626
Line 626: // TODO(ARM) Is this necessary? Is the page size correct?
I think we can just delete this on ARM.

http://codereview.chromium.org/173567/diff/1/9#newcode640
Line 640: // Load previous char as initial value of current-character.
No-need for a hyphen-here.

http://codereview.chromium.org/173567/diff/1/9#newcode642
Line 642: __ ldrb(r0, MemOperand(frame_pointer(), kAtStart));
ldr

http://codereview.chromium.org/173567/diff/1/9#newcode664
Line 664: __ add(r2, r2, Operand(r1));
This add and mov can be combined into one instruction! (And given that the
number of registers is always even perhaps we should unwind once).

http://codereview.chromium.org/173567/diff/1/9#newcode786
Line 786: bool check_bounds,
We don't use check_bounds.  Can that be right?

http://codereview.chromium.org/173567/diff/1/9#newcode803
Line 803: }
This is as far as I got.

http://codereview.chromium.org/173567/diff/1/9#newcode1056
Line 1056: __ mov(r0, Operand(stack_guard_limit));
While you were working on this patch we made a faster way to do preemption
checks.  Probably best to fix in a second patch unless you already moved up to
the latest version before starting the gcl issue.

[Lasse Reichstein, 2009-08-28 09:15:17]

http://codereview.chromium.org/173567/diff/1/4
File src/arm/assembler-arm.cc (right):

http://codereview.chromium.org/173567/diff/1/4#newcode342
Line 342: return pos + 8 + imm26;
Changed to a constant, kPcLoadDelta. It's the difference between the address of
the current opcode and the value that the opcode would read from the pc
register.

http://codereview.chromium.org/173567/diff/1/4#newcode355
Line 355: int imm26 = target_pos - pos - 8;
Ditto.

http://codereview.chromium.org/173567/diff/1/4#newcode383
Line 383: PrintF("value");
It does need a newline. It can't really print a value, since it's an unbound
label's use chain that is being traversed. It has no value yet.

http://codereview.chromium.org/173567/diff/1/4#newcode688
Line 688: return target_pos - pc_offset() - 8;
And this one, too.

http://codereview.chromium.org/173567/diff/1/7
File src/arm/constants-arm.h (right):

http://codereview.chromium.org/173567/diff/1/7#newcode52
Line 52: #include "disasm.h"
Whoop. It's a leftover from something added while debugging. It isn't used, so
it's removed.

http://codereview.chromium.org/173567/diff/1/9
File src/arm/regexp-macro-assembler-arm.cc (right):

http://codereview.chromium.org/173567/diff/1/9#newcode85
Line 85: * int (*match)(String* input_string,
It's in NativeRegExpMacroAssembler::Execute in regexp-macro-assembler.cc. I'll
add a reference.

http://codereview.chromium.org/173567/diff/1/9#newcode140
Line 140: void RegExpMacroAssemblerARM::AdvanceRegister(int reg, int by) {
No real need. It's at a fixed offset from the frame pointer, so we can load it
in one ldr in any case.

http://codereview.chromium.org/173567/diff/1/9#newcode155
Line 155: __ add(pc, r0, Operand(masm_->CodeObject()));
Good point. It might have the same number of opcodes, but due to the memory
read, it will be slower.

http://codereview.chromium.org/173567/diff/1/9#newcode182
Line 182: __ ldrb(r0, MemOperand(frame_pointer(), kAtStart));
Done.

http://codereview.chromium.org/173567/diff/1/9#newcode187
Line 187: __ add(r0, end_of_input_address(), Operand(current_input_offset()));
Done

http://codereview.chromium.org/173567/diff/1/9#newcode201
Line 201: __ add(r0, end_of_input_address(), Operand(current_input_offset()));
Done

http://codereview.chromium.org/173567/diff/1/9#newcode238
Line 238: __ cmp(r1, Operand(str[i]));
String-in-constant-pool sounds like something the security people would barf
over (it's executable after all :).
I'll load low and high bytes separately (if necessary), and only load the high
bits if necessary. It will take an extra opcode, but it goes between the load
and the compare, so timing should be fine.

http://codereview.chromium.org/173567/diff/1/9#newcode308
Line 308: // Restore original values before failing.
Mis-retained from the ia32 version. We didn't need to save any registers in the
ARM code.

http://codereview.chromium.org/173567/diff/1/9#newcode546
Line 546: __ mov(r0, Operand(0));
Done.

http://codereview.chromium.org/173567/diff/1/9#newcode615
Line 615: // page (a problem on Windows).
Gone.

http://codereview.chromium.org/173567/diff/1/9#newcode626
Line 626: // TODO(ARM) Is this necessary? Is the page size correct?
Done.

http://codereview.chromium.org/173567/diff/1/9#newcode664
Line 664: __ add(r2, r2, Operand(r1));
Done (both combining the add and move, and unrolling).

http://codereview.chromium.org/173567/diff/1/9#newcode786
Line 786: bool check_bounds,
Doesn't seem so. We should only call CheckPosition if check_bounds is true.
Fixed both here and on ia32/x64.

[Erik Corry, 2009-08-28 09:47:02]

LGTM though some of the comments seem to indicate that it's not quite done...

http://codereview.chromium.org/173567/diff/1/9
File src/arm/regexp-macro-assembler-arm.cc (right):

http://codereview.chromium.org/173567/diff/1/9#newcode809
Line 809: __ mov(r0, Operand(target + Code::kHeaderSize));
If the label is used in a constant pool address that is within range we should
load from there (unless we can encode the number in one instruction).  This
instruction will otherwise create a new constant pool address.  If it isn't in
range we should make a new constant pool address and put it in the label. 
Perhaps we need a subclass of Label for this.

http://codereview.chromium.org/173567/diff/1/9#newcode933
Line 933: UNIMPLEMENTED();  // TODO(ARM): Can't handle moving code yet.
We need to fix this!

http://codereview.chromium.org/173567/diff/1/9#newcode1026
Line 1026: __ add(lr, lr, Operand(masm_->CodeObject()));
Can't we just add(pc, lr...

http://codereview.chromium.org/173567/diff/1/9#newcode1106
Line 1106: // Make this work!
Yes!

http://codereview.chromium.org/173567/diff/1/9#newcode1132
Line 1132: if (OS::ActivationFrameAlignment() != 0) {
> 4

http://codereview.chromium.org/173567/diff/1/9#newcode1163
Line 1163: if (characters == 4) {
This is unnecessary.  On a system that can't load unaligned we are never asked
to load more than one character at a time.

http://codereview.chromium.org/173567/diff/1/16
File src/execution.h (right):

http://codereview.chromium.org/173567/diff/1/16#newcode31
Line 31: #include "v8.h"
Not needed.

http://codereview.chromium.org/173567/diff/1/18
File src/heap.h (right):

http://codereview.chromium.org/173567/diff/1/18#newcode138
Line 138: #if V8_TARGET_ARCH_ARM && V8_NATIVE_REGEXP
Ugh

http://codereview.chromium.org/173567/diff/1/18#newcode1034
Line 1034: #if V8_TARGET_ARCH_ARM && V8_NATIVE_REGEXP
Can't we remove the #if here?

http://codereview.chromium.org/173567/diff/1/28
File src/serialize.cc (right):

http://codereview.chromium.org/173567/diff/1/28#newcode740
Line 740: 17,
Seems like you shouldn't be reusing 17 here.

[Lasse Reichstein, 2009-08-28 11:43:15]

http://codereview.chromium.org/173567/diff/1/9
File src/arm/regexp-macro-assembler-arm.cc (right):

http://codereview.chromium.org/173567/diff/1/9#newcode809
Line 809: __ mov(r0, Operand(target + Code::kHeaderSize));
It's an interesting idea, but I'll prefer to look at the code before doing a
complex rewrite to achieve this.

http://codereview.chromium.org/173567/diff/1/9#newcode933
Line 933: UNIMPLEMENTED();  // TODO(ARM): Can't handle moving code yet.
Fixed. Removed the UNIMPLEMENTED and comment. It should work.

http://codereview.chromium.org/173567/diff/1/9#newcode1026
Line 1026: __ add(lr, lr, Operand(masm_->CodeObject()));
Should work. Done.

http://codereview.chromium.org/173567/diff/1/9#newcode1056
Line 1056: __ mov(r0, Operand(stack_guard_limit));
If this still works, I'll leave it for a second patch.

http://codereview.chromium.org/173567/diff/1/9#newcode1106
Line 1106: // Make this work!
Comment removed.

http://codereview.chromium.org/173567/diff/1/9#newcode1132
Line 1132: if (OS::ActivationFrameAlignment() != 0) {
Done

http://codereview.chromium.org/173567/diff/1/9#newcode1163
Line 1163: if (characters == 4) {
I'll assert that characters is 1, and hope the calling code knows how to handle
it.

http://codereview.chromium.org/173567/diff/1/18
File src/heap.h (right):

http://codereview.chromium.org/173567/diff/1/18#newcode138
Line 138: #if V8_TARGET_ARCH_ARM && V8_NATIVE_REGEXP
Agree, but it still seems better than having an unused strong root in the other
cases.

http://codereview.chromium.org/173567/diff/1/18#newcode1034
Line 1034: #if V8_TARGET_ARCH_ARM && V8_NATIVE_REGEXP
Sure. We still won't implement the function unless we are using native regexps
on ARM.

http://codereview.chromium.org/173567/diff/1/28
File src/serialize.cc (right):

http://codereview.chromium.org/173567/diff/1/28#newcode740
Line 740: 17,
Whoops.