Capture register clearing on loop iteration

Added clearing of captures before entering the body of a loop.  This
also revealed a bug or two that had to be fixed.

[Lasse Reichstein, 2009-01-14 08:38:34]

(I have only looked at the ia32 code)

http://codereview.chromium.org/17378/diff/1/6
File src/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/17378/diff/1/6#newcode932
Line 932: __ mov(register_location(reg), esi);
The esi register does not hold the needed value (one character before the start
of the string) but instead it points to the end of the string.
A cleared register would be seen as captured, with a position at the end of the
string.

Code that, I think, should trigger this:
 var m = /(?:(a)|b)*/.exec("ab");
 // m[1] should be null, it is "".
Otherwise a C-level test should show the difference in returned value for the
capture registers (2 instead of -1).

The correct value is computed in GetCode(), in the entry part, where we
initialize the capture registers. I suggest storing it somewhere safe (e.g., as
a "local variable" in the stack frame) until it's needed.

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

http://codereview.chromium.org/17378/diff/1/9#newcode69
Line 69: virtual void ClearRegister(int reg);
Could perhaps be called "ClearPositionRegister", because the value it resets to
is the -1 position. It does not make sense for, e.g., registers used as
counters.

[Erik Corry, 2009-01-14 09:24:31]

LGTM

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

http://codereview.chromium.org/17378/diff/1/4#newcode1378
Line 1378: assembler->PushRegister(reg, check_stack_limit);
It's a waste to push...

http://codereview.chromium.org/17378/diff/1/4#newcode1389
Line 1389: if (affected_registers.Get(reg)) assembler->PopRegister(reg);
... and pop registers if they are just affected by a WritePosition.  We could
just clear them instead of popping them.

http://codereview.chromium.org/17378/diff/1/4#newcode1428
Line 1428: case ActionNode::STORE_POSITION: {
We should be ignoring a store position that we find after we found a clear...

http://codereview.chromium.org/17378/diff/1/4#newcode1438
Line 1438: case ActionNode::CLEAR_CAPTURES: {
...and we should be ignoring a clear that we find after a store position.

http://codereview.chromium.org/17378/diff/1/4#newcode3661
Line 3661: } else if (!needs_capture_clearing) {
Surely it's OK to unroll and no need to clear captures if max is 1?

http://codereview.chromium.org/17378/diff/1/5
File src/jsregexp.h (right):

http://codereview.chromium.org/17378/diff/1/5#newcode717
Line 717: RegExpNode* on_success);
This fits on one line.

http://codereview.chromium.org/17378/diff/1/5#newcode764
Line 764: int range_from;
Can't we just have an Interval here?

http://codereview.chromium.org/17378/diff/1/5#newcode1011
Line 1011: bool Mentions(int reg);
It's messy that this is not a private method, since it returns an invalid
register for some actions.  How about a public method that returns a (perhaps
trivial) Interval instead?

[Erik Corry, 2009-01-14 09:30:13]

in addition...

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

http://codereview.chromium.org/17378/diff/1/4#newcode1389
Line 1389: if (affected_registers.Get(reg)) assembler->PopRegister(reg);
On 2009/01/14 09:24:31, Erik Corry wrote:
> ... and pop registers if they are just affected by a WritePosition.  We could
> just clear them instead of popping them.

In fact for register 0 there's no need to clear it either.  Either it will be
overwritten later or the regexp doesn't match in which case it doesn't matter
what is in it.

[Christian Plesner Hansen, 2009-01-14 11:31:35]

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

http://codereview.chromium.org/17378/diff/1/4#newcode1378
Line 1378: assembler->PushRegister(reg, check_stack_limit);
I'd like to defer that optimization.

http://codereview.chromium.org/17378/diff/1/4#newcode1428
Line 1428: case ActionNode::STORE_POSITION: {
Fixed

http://codereview.chromium.org/17378/diff/1/4#newcode1438
Line 1438: case ActionNode::CLEAR_CAPTURES: {
Fixed

http://codereview.chromium.org/17378/diff/1/5
File src/jsregexp.h (right):

http://codereview.chromium.org/17378/diff/1/5#newcode717
Line 717: RegExpNode* on_success);
Fixed

http://codereview.chromium.org/17378/diff/1/5#newcode764
Line 764: int range_from;
I thought so too but it turns out that you can't have union members that have a
constructor.

http://codereview.chromium.org/17378/diff/1/5#newcode1011
Line 1011: bool Mentions(int reg);
I'm not sure -- all other operations than the one I added use just one register
and it would make that code more cluttered to use intervals there.

http://codereview.chromium.org/17378/diff/1/6
File src/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/17378/diff/1/6#newcode932
Line 932: __ mov(register_location(reg), esi);
Fixed

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

http://codereview.chromium.org/17378/diff/1/9#newcode69
Line 69: virtual void ClearRegister(int reg);
Good point.