Implement loop for global regexps in regexp assembler. (issue 10386090)

[yan...@chromium.org]

Reviewers: Erik Corry,

Message:
PTAL.

This CL is far from being completed since platform ports and tests are
missing.
I still want to get a second opinion on this before I continue.

I implemented the loop inside GetCode() that deals with setting up local
variables and returning results. I initially thought about doing it in a
higher
level way, representing the loop as a RegExpNode, but then changes to
Fail() and
Succeed() were also necessary, so I chose doing it in low-level assembly
instead. Suggestions on that are very welcome.

Description:
Implement loop for global regexps in regexp assembler.

BUG=
TEST=


Please review this at http://codereview.chromium.org/10386090/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/ia32/code-stubs-ia32.cc
M src/ia32/regexp-macro-assembler-ia32.h
M src/ia32/regexp-macro-assembler-ia32.cc
M src/ia32/simulator-ia32.h
M src/jsregexp.h
M src/jsregexp.cc
M src/regexp-macro-assembler-tracer.cc
M src/regexp-macro-assembler.h
M src/regexp-macro-assembler.cc
M src/runtime.cc
M test/cctest/test-regexp.cc

[erik....@gmail.com]

http://codereview.chromium.org/10386090/diff/1/src/ia32/regexp-macro-assembler-ia32.h
File src/ia32/regexp-macro-assembler-ia32.h (right):

http://codereview.chromium.org/10386090/diff/1/src/ia32/regexp-macro-assembler-ia32.h#newcode139
src/ia32/regexp-macro-assembler-ia32.h:139: // For the case of global
regular expression, we have room to store more
more than one -> at least one
?

http://codereview.chromium.org/10386090/diff/1/src/ia32/regexp-macro-assembler-ia32.h#newcode160
src/ia32/regexp-macro-assembler-ia32.h:160: // The highest bit on the
output registers count signals success. This
lowest bit?

http://codereview.chromium.org/10386090/diff/1/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/10386090/diff/1/src/jsregexp.cc#newcode200
src/jsregexp.cc:200: if (parse_result.simple &&
It would be interesting to remove this stuff so we never AtomCompile,
but always use the regexp engine. With these changes it might be
faster, and it would simplify things a lot.

http://codereview.chromium.org/10386090/diff/1/src/jsregexp.cc#newcode904
src/jsregexp.cc:904: RegExpCompiler(int capture_count,
You don't need this change.

http://codereview.chromium.org/10386090/diff/1/src/jsregexp.cc#newcode981
src/jsregexp.cc:981: bool ignore_case,
You don't need this change.

http://codereview.chromium.org/10386090/diff/1/src/jsregexp.cc#newcode5796
src/jsregexp.cc:5796: RegExpCompiler compiler(data->capture_count,
Or this.

http://codereview.chromium.org/10386090/diff/1/src/regexp-macro-assembler.h
File src/regexp-macro-assembler.h (right):

http://codereview.chromium.org/10386090/diff/1/src/regexp-macro-assembler.h#newcode186
src/regexp-macro-assembler.h:186: // Set wether the regular expression
has the global flag. Failures in a
A wether is a castrated billy-goat :-)

http://codereview.chromium.org/10386090/diff/1/src/regexp-macro-assembler.h#newcode187
src/regexp-macro-assembler.h:187: // global regexp may still mean
success overal.
overall.

http://codereview.chromium.org/10386090/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/10386090/diff/1/src/runtime.cc#newcode3798
src/runtime.cc:3798: static int SearchRegExpNoCaptureMultiple(
I wonder if we can unify this and SearchRegExpMultiple without a
performance hit.

http://codereview.chromium.org/10386090/diff/1/src/runtime.cc#newcode3818
src/runtime.cc:3818: int result =
How about giving this variable a name like number_of_matches, and
getting rid of RE_SUCCESS and RE_FAILURE. Just comparing == 0 and > 0
makes sense if the variable has the right name.

http://codereview.chromium.org/10386090/diff/1/src/runtime.cc#newcode3851
src/runtime.cc:3851: if (match_start != match_end) {
I don't see this logic replicated in the regexp-macro-assembler. This
suggests to me that we are missing good tests for it.

http://codereview.chromium.org/10386090/diff/1/src/runtime.cc#newcode3855
src/runtime.cc:3855: if (pos > subject_length) {
unneeded edit

http://codereview.chromium.org/10386090/diff/1/src/runtime.cc#newcode3940
src/runtime.cc:3940: match =
isolate->factory()->NewProperSubString(subject,
Not sure why we need NewProperSubString here.

http://codereview.chromium.org/10386090/diff/1/src/runtime.cc#newcode3949
src/runtime.cc:3949: for (int i = 1; i <= capture_count; i++) {
I think you could just start this loop at 0 and remove some code
duplication.

http://codereview.chromium.org/10386090/