Building on regexp-ia32.

IA32 Code generation from regexps - incomplete.
Merged with latest changes for RegExp Macro Assembler interface.

[Lasse Reichstein, 2008-11-19 13:48:09]

Committed progress on IA32 regexp code generation.
Review, please.

[Erik Corry, 2008-11-21 13:03:04]

LGTM - good work.  Can't wait to see how it works in practice.

I haven't checked all the assembler output for correctness.  We need to get some
tests running on this stuff.

http://codereview.chromium.org/11271/diff/201/21
File regexp2000/src/SConscript (right):

http://codereview.chromium.org/11271/diff/201/21#newcode46
Line 46: 'property.cc', 'regexp-macro-assembler.cc',
Please reformat.

http://codereview.chromium.org/11271/diff/201/23
File regexp2000/src/assembler-ia32.h (right):

http://codereview.chromium.org/11271/diff/201/23#newcode446
Line 446: void push(Label* label, RelocInfo::Mode relocation_mode =
RelocInfo::NONE);
This should be without a default argument to help overloading resolution.  We
use default arguments very sparingly (where a call is frequently used and the
default is blindingly obvious to any user.

http://codereview.chromium.org/11271/diff/201/27
File regexp2000/src/heap.cc (right):

http://codereview.chromium.org/11271/diff/201/27#newcode1628
Line 1628: Handle<Code>* self) {
As discussed offline this should be passed by value and it needs to be hooked
into Factory::NewCode.

http://codereview.chromium.org/11271/diff/201/27#newcode1655
Line 1655: code->CopyFrom(desc);  // migrate generated code
A comment here to the effect that CopyFrom may make use of the location of the
self handle when relocating when moving code into the code object.

http://codereview.chromium.org/11271/diff/201/28
File regexp2000/src/heap.h (right):

http://codereview.chromium.org/11271/diff/201/28#newcode563
Line 563: Handle<Code>* self = NULL);
We shouldn't mix handle and non-handle code.  This should probably be a Code**. 
A comment as to it's meaning would be a good idea here.

http://codereview.chromium.org/11271/diff/201/29
File regexp2000/src/jsregexp.cc (right):

http://codereview.chromium.org/11271/diff/201/29#newcode2295
Line 2295: if (FLAG_re2k_native) {
We should probably either ignore the flag on ARM or not have it at all.  This
looks like it will crash in release mode if the flag is set on ARM?

http://codereview.chromium.org/11271/diff/201/29#newcode2301
Line 2301: // FIXME.
Thuis should be a TODO with a name or issue number.

http://codereview.chromium.org/11271/diff/201/29#newcode2302
Line 2302: // Don't compile *here*, we don't know the input type yet! This won't
work!
For now we just do 16 bit always for the bytecode version.

http://codereview.chromium.org/11271/diff/201/30
File regexp2000/src/objects.h (right):

http://codereview.chromium.org/11271/diff/201/30#newcode2120
Line 2120: // No more than eight kinds. The value currently encoded in three
bits in
Please add an assert somewhere.

http://codereview.chromium.org/11271/diff/201/31
File regexp2000/src/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/11271/diff/201/31#newcode53
Line 53: * The stack is expected to have the following structure (tentative):
Be bold, not tentative! :-)

http://codereview.chromium.org/11271/diff/201/31#newcode347
Line 347: NULL,
Indentation!

http://codereview.chromium.org/11271/diff/201/31#newcode414
Line 414: __ j(above, &no_preempt);
We don't have to fix this now, but this needs moving out of line.  We push
backtracks a lot and this will cause too much bloat.

http://codereview.chromium.org/11271/diff/201/31#newcode469
Line 469: __ jmp(&success_label_);
Do we need to set eax here?

http://codereview.chromium.org/11271/diff/201/35
File regexp2000/test/cctest/test-regexp.cc (right):

http://codereview.chromium.org/11271/diff/201/35#newcode277
Line 277: const char* kNothingToRepeat = "Nothing to repeat";
Surely 'static const'?

[Lasse Reichstein, 2008-11-24 08:32:33]

Addressed comments, but I have also managed to break some tests in the process.

http://codereview.chromium.org/11271/diff/201/21
File regexp2000/src/SConscript (right):

http://codereview.chromium.org/11271/diff/201/21#newcode46
Line 46: 'property.cc', 'regexp-macro-assembler.cc',
Done.
Wouldn't it be easier to maintain if the file names were a vertical list
instead?

http://codereview.chromium.org/11271/diff/201/23
File regexp2000/src/assembler-ia32.h (right):

http://codereview.chromium.org/11271/diff/201/23#newcode446
Line 446: void push(Label* label, RelocInfo::Mode relocation_mode =
RelocInfo::NONE);
Indeed this actually hid an error where push(0) was taken to mean this push.
Done.

http://codereview.chromium.org/11271/diff/201/27
File regexp2000/src/heap.cc (right):

http://codereview.chromium.org/11271/diff/201/27#newcode1628
Line 1628: Handle<Code>* self) {
On 2008/11/21 13:03:04, Erik Corry wrote:
> As discussed offline this should be passed by value and it needs to be hooked
> into Factory::NewCode.

Done.

http://codereview.chromium.org/11271/diff/201/27#newcode1655
Line 1655: code->CopyFrom(desc);  // migrate generated code
On 2008/11/21 13:03:04, Erik Corry wrote:
> A comment here to the effect that CopyFrom may make use of the location of
the
> self handle when relocating when moving code into the code object.

Done.

http://codereview.chromium.org/11271/diff/201/28
File regexp2000/src/heap.h (right):

http://codereview.chromium.org/11271/diff/201/28#newcode563
Line 563: Handle<Code>* self = NULL);
On 2008/11/21 13:03:04, Erik Corry wrote:
> We shouldn't mix handle and non-handle code.  This should probably be a
Code**. 
> A comment as to it's meaning would be a good idea here.

Done.

http://codereview.chromium.org/11271/diff/201/29
File regexp2000/src/jsregexp.cc (right):

http://codereview.chromium.org/11271/diff/201/29#newcode2295
Line 2295: if (FLAG_re2k_native) {
On 2008/11/21 13:03:04, Erik Corry wrote:
> We should probably either ignore the flag on ARM or not have it at all.  This
> looks like it will crash in release mode if the flag is set on ARM?

Done.

http://codereview.chromium.org/11271/diff/201/29#newcode2301
Line 2301: // FIXME.
On 2008/11/21 13:03:04, Erik Corry wrote:
> Thuis should be a TODO with a name or issue number.

Done.

http://codereview.chromium.org/11271/diff/201/29#newcode2302
Line 2302: // Don't compile *here*, we don't know the input type yet! This won't
work!
The ia32 code will also assume 16-bit strings then, for now.

http://codereview.chromium.org/11271/diff/201/30
File regexp2000/src/objects.h (right):

http://codereview.chromium.org/11271/diff/201/30#newcode2120
Line 2120: // No more than eight kinds. The value currently encoded in three
bits in
On 2008/11/21 13:03:04, Erik Corry wrote:
> Please add an assert somewhere.

Done.

http://codereview.chromium.org/11271/diff/201/31
File regexp2000/src/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/11271/diff/201/31#newcode53
Line 53: * The stack is expected to have the following structure (tentative):
Done!

http://codereview.chromium.org/11271/diff/201/31#newcode347
Line 347: NULL,
On 2008/11/21 13:03:04, Erik Corry wrote:
> Indentation!
> 

Done.

http://codereview.chromium.org/11271/diff/201/31#newcode469
Line 469: __ jmp(&success_label_);
No.
The code at success_lable_ sets eax and runs into the exit code. Fail sets eax
and jumps to the exit code.

http://codereview.chromium.org/11271/diff/201/35
File regexp2000/test/cctest/test-regexp.cc (right):

http://codereview.chromium.org/11271/diff/201/35#newcode277
Line 277: const char* kNothingToRepeat = "Nothing to repeat";
On 2008/11/21 13:03:04, Erik Corry wrote:
> Surely 'static const'?

Why? 
None of the other char*'s are static. 
Wouldn't it be a vaste of static memory?