Integrate the Irregexp Regular Expression Engine.

Integrate the Irregexp Regular Expression Engine.

This rebases the original irregexp port [1] on top our copy of V8 r24065 [2].

[1]: https://codereview.chromium.org/539153002/
[2]: https://codereview.chromium.org/678193004/

BUG=

[zerny-google, 2014-10-28 08:28:35]

zerny@google.com changed reviewers:
+ fschneider@google.com, iposva@google.com, vegorov@google.com

[zerny-google, 2014-10-28 08:34:06]

Rebased Jakob's CL on the latest V8. PTAL.

Once the tests are landed, I'll rebase this to remove the VM/Skip in
corelib.status

V8 file copy CL: https://codereview.chromium.org/678193004/
Test port CL: https://codereview.chromium.org/667043003/

[Ivan Posva, 2014-10-31 07:32:38]

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp.cc
File runtime/lib/regexp.cc (right):

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp.cc#ne...
runtime/lib/regexp.cc:29: #if defined(USE_JSCRE)
Let's figure out whether this is even needed.

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp_patch...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp_patch...
runtime/lib/regexp_patch.dart:37: // TODO(jgruber): We might not want to
canonicalize regexp objects.
Here and other places "TODO(zerny)".

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/string_patch...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:757: codeUnits |= this.codeUnitAt(index + i) << (i
* 8);
This looks pretty endian specific to me. No?

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:1039: codeUnits |= this.codeUnitAt(index + i) <<
(i * 16);
ditto.

[zerny-google, 2014-10-31 11:54:26]

Thanks for the comments.

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp.cc
File runtime/lib/regexp.cc (right):

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp.cc#ne...
runtime/lib/regexp.cc:29: #if defined(USE_JSCRE)
On 2014/10/31 07:32:37, Ivan Posva wrote:
> Let's figure out whether this is even needed.

Would it be of more value to replace this with a VM flag? Otherwise, I'll gladly
remove JSCRE completely.

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp_patch...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/regexp_patch...
runtime/lib/regexp_patch.dart:37: // TODO(jgruber): We might not want to
canonicalize regexp objects.
On 2014/10/31 07:32:37, Ivan Posva wrote:
> Here and other places "TODO(zerny)".

Done.

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/string_patch...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/683433003/diff/80001/runtime/lib/string_patch...
runtime/lib/string_patch.dart:757: codeUnits |= this.codeUnitAt(index + i) << (i
* 8);
On 2014/10/31 07:32:37, Ivan Posva wrote:
> This looks pretty endian specific to me. No?

Well spotted. The irregexp code works despite this because we have no big-endian
systems that use this code-unit packing (mips does not support unaligned reads).

After some discussion with Florian, we have decided to avoid this dart code and
instead make it native and generate IR instructions instead. The endian issue
remains however between the current C++ code and the assembly code. I'll either
fix that or assert that packing is not used on big-endian systems.

[zerny-google, 2014-10-31 15:01:39]

> After some discussion with Florian, we have decided to avoid this dart code
and
> instead make it native and generate IR instructions instead. The endian issue
> remains however between the current C++ code and the assembly code. I'll
either
> fix that or assert that packing is not used on big-endian systems.

I've uploaded a new patch which uses LoadCodeUnitIntr in the unoptimizing pass
and thus eliminates all of the special casing related to *CodeUnitsAt. The
irregexp code is still not safe for execution on a bigendian system that uses
preloading, such a setup is however not possible atm. For good measure, I'll add
code to explicitly disallow preloading on bigendian systems after the weekend.

[zerny-google, 2014-11-03 08:48:32]

Rebased properly on top of the "V8 copy" CL. The latest patch set now shows the
change done to the already added V8 files.

[Ivan Posva, 2014-11-03 08:52:21]

Some more comments as discussed in person already.

Also I would prefer to either make the switch between regular expression engines
via flag or not at all. Are you confident that the irregexp based solution is a
par to the JSCRE regexp handling in the VM currently? If so I suggest to drop
support for the old engine entirely.

-Ivan

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5463: 
two lines

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5465: StoreNonPointer(&raw_ptr()->regexp_cid_, regexp_cid);
The cid should be asserted to be a string only cid.

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5467: 
two lines

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6147: result.set_deoptimization_counter(0);
cid value is not being initialized here and similar places below.

We should also find a good spot where we assert that the value is properly
initialized for non-irregexp functions and within String cid range for irregexp
functions.

[zerny-google, 2014-11-03 14:03:41]

Thanks for the new round of comments. I've added FLAG_use_jscre with default
value 'false' and checked that the system still completes all tests (when
reverting the irregexp test changes).

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5463: 
On 2014/11/03 08:52:21, Ivan Posva wrote:
> two lines

Done.

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5465: StoreNonPointer(&raw_ptr()->regexp_cid_, regexp_cid);
On 2014/11/03 08:52:21, Ivan Posva wrote:
> The cid should be asserted to be a string only cid.

Done.

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5467: 
On 2014/11/03 08:52:21, Ivan Posva wrote:
> two lines

Done.

https://codereview.chromium.org/683433003/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6147: result.set_deoptimization_counter(0);
On 2014/11/03 08:52:21, Ivan Posva wrote:
> cid value is not being initialized here and similar places below.
> 
> We should also find a good spot where we assert that the value is properly
> initialized for non-irregexp functions and within String cid range for
irregexp
> functions.

Done.

[Ivan Posva, 2014-11-05 07:55:10]

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5453: ASSERT(obj.IsJSRegExp());
The Cast below is executing this assertion exactly. A verified cast (in debug)
is why we added that Cast function.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5107: Function::New(String::Handle(isolate,
Symbols::New("RegExp")),
Please predefine this symbol.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5161: lib.LookupClass(String::Handle(isolate,
Symbols::New("RegExp"))));
diito for the symbol

But the question is really why we are "adding" the irregexp functions to the
RegExp class.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.h
File runtime/vm/regexp.h (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.h#new...
runtime/vm/regexp.h:1404: const String& pattern,
Weird indent.

[Ivan Posva, 2014-11-05 07:56:31]

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:224: const intptr_t num_params =
function().num_fixed_parameters();;
As far as I understand this code, the number of fix parameters is a constant.
Can't you assert that here?

[zerny-google, 2014-11-05 11:52:00]

Thanks for the comments!

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5453: ASSERT(obj.IsJSRegExp());
On 2014/11/05 07:55:09, Ivan Posva wrote:
> The Cast below is executing this assertion exactly. A verified cast (in debug)
> is why we added that Cast function.

Removed the assert.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/parser.cc#ne...
runtime/vm/parser.cc:224: const intptr_t num_params =
function().num_fixed_parameters();;
On 2014/11/05 07:56:30, Ivan Posva wrote:
> As far as I understand this code, the number of fix parameters is a constant.
> Can't you assert that here?

Yes. Added the assert.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5107: Function::New(String::Handle(isolate,
Symbols::New("RegExp")),
On 2014/11/05 07:55:09, Ivan Posva wrote:
> Please predefine this symbol.

Done.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5161: lib.LookupClass(String::Handle(isolate,
Symbols::New("RegExp"))));
On 2014/11/05 07:55:09, Ivan Posva wrote:
> diito for the symbol
> 
> But the question is really why we are "adding" the irregexp functions to the
> RegExp class.

AFAIK we don't allow a non-existent owner, so setting the RegExp class as the
owner seems natural. I can see some other generated functions use
Type::Function().type_class(). Would that be better?

Access to the functions only takes place through the JSRegExp object.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.h
File runtime/vm/regexp.h (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.h#new...
runtime/vm/regexp.h:1404: const String& pattern,
On 2014/11/05 07:55:09, Ivan Posva wrote:
> Weird indent.

Done.

[Ivan Posva, 2014-11-07 08:46:54]

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5107: Function::New(String::Handle(isolate,
Symbols::New("RegExp")),
On 2014/11/05 11:52:00, zerny-google wrote:
> On 2014/11/05 07:55:09, Ivan Posva wrote:
> > Please predefine this symbol.
> 
> Done.

Actually looking at this a bit more "RegExp" is really a bad name for these
functions as it is too close to the constructor name and will like lead to
confusion at some point. A suggestion that I can come up with immediately is to
name this function ":irregexp" which is an illegal identifier and thus ensures
that these functions cannot be called directly from Dart code. See for example
Function::NewEvalFunction.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5161: lib.LookupClass(String::Handle(isolate,
Symbols::New("RegExp"))));
On 2014/11/05 11:52:00, zerny-google wrote:
> On 2014/11/05 07:55:09, Ivan Posva wrote:
> > diito for the symbol
> > 
> > But the question is really why we are "adding" the irregexp functions to the
> > RegExp class.
> 
> AFAIK we don't allow a non-existent owner, so setting the RegExp class as the
> owner seems natural. I can see some other generated functions use
> Type::Function().type_class(). Would that be better?
> 
> Access to the functions only takes place through the JSRegExp object.

I was considering whether it would simplify things if we made these closures
instead of functions as they are stored in instance fields, but came to the
realization that it would not solve any of the naming and hook up to owner
issues.

[Ivan Posva, 2014-11-10 08:16:24]

Next round of questions...
-Ivan

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/flow_graph_r...
File runtime/vm/flow_graph_range_analysis.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.cc:2835:
RangeBoundary::FromConstant(kMaxUint32));
Would we ever be loading 8 bytes at a time on 64-bit architectures?

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:1293: void
AddIndirectEntry(IndirectEntryInstr* entry) {
It is very unclear what these entries are used for and why they should be on the
GraphEntry instruction.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2191: // entry consisting of a goto to the
final indirect entry is inserted on
This comment is unclear. What is "final" in this context? The last indirect
entry from the offset/successors array?

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2243: GrowableArray<TargetEntryInstr*>
successors_;
How many of these successors and corresponding offsets do we get for each
IndirectGotoInstr?

[zerny-google, 2014-11-10 09:10:42]

Thanks again. Answers are inlined below.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/flow_graph_r...
File runtime/vm/flow_graph_range_analysis.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.cc:2835:
RangeBoundary::FromConstant(kMaxUint32));
On 2014/11/10 08:16:23, Ivan Posva wrote:
> Would we ever be loading 8 bytes at a time on 64-bit architectures?

No. The current LoadCodeUnitsInstr will pack at most 4 bytes (4x8 or 2x16)
regardless of architecture.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:1293: void
AddIndirectEntry(IndirectEntryInstr* entry) {
On 2014/11/10 08:16:23, Ivan Posva wrote:
> It is very unclear what these entries are used for and why they should be on
the
> GraphEntry instruction.

As far as I understand, the graph entry is the main entry of any graph (and the
only entry we know exists for any graph). The indirect (join) entries are needed
to represent the possible backtracking points in a way that satisfies the flow
graph assumptions. We add the entries to a known place (ie, the graph entry) so
we can finalize the construction by linking up the jumps with the joins after
graph construction.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2191: // entry consisting of a goto to the
final indirect entry is inserted on
On 2014/11/10 08:16:23, Ivan Posva wrote:
> This comment is unclear. What is "final" in this context? The last indirect
> entry from the offset/successors array?

I'll update the comment to be more clear.

What this comment is trying to say is that an "indirect" goto represents a
dynamically computed goto. In order to represent the CFG of this goto we must
link it to each of the possible targets it might jump to without breaking the
assumptions of the flow graph representation. Thus the dynamic jump is
represented as an "IndirectGotoInstr" that for each possible target contains a
successor "GotoInstr" that jumps to the actual target "IndirectEntryInstr".

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2243: GrowableArray<TargetEntryInstr*>
successors_;
On 2014/11/10 08:16:23, Ivan Posva wrote:
> How many of these successors and corresponding offsets do we get for each
> IndirectGotoInstr?

One for each possible target. The number of targets depends on how many
backtracking points exist in the generated code for a particular regexp matching
function.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5107: Function::New(String::Handle(isolate,
Symbols::New("RegExp")),
On 2014/11/07 08:46:54, Ivan Posva wrote:
> On 2014/11/05 11:52:00, zerny-google wrote:
> > On 2014/11/05 07:55:09, Ivan Posva wrote:
> > > Please predefine this symbol.
> > 
> > Done.
> 
> Actually looking at this a bit more "RegExp" is really a bad name for these
> functions as it is too close to the constructor name and will like lead to
> confusion at some point. A suggestion that I can come up with immediately is
to
> name this function ":irregexp" which is an illegal identifier and thus ensures
> that these functions cannot be called directly from Dart code. See for example
> Function::NewEvalFunction.

Done.

https://codereview.chromium.org/683433003/diff/200001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:5161: lib.LookupClass(String::Handle(isolate,
Symbols::New("RegExp"))));
On 2014/11/07 08:46:54, Ivan Posva wrote:
> On 2014/11/05 11:52:00, zerny-google wrote:
> > On 2014/11/05 07:55:09, Ivan Posva wrote:
> > > diito for the symbol
> > > 
> > > But the question is really why we are "adding" the irregexp functions to
the
> > > RegExp class.
> > 
> > AFAIK we don't allow a non-existent owner, so setting the RegExp class as
the
> > owner seems natural. I can see some other generated functions use
> > Type::Function().type_class(). Would that be better?
> > 
> > Access to the functions only takes place through the JSRegExp object.
> 
> I was considering whether it would simplify things if we made these closures
> instead of functions as they are stored in instance fields, but came to the
> realization that it would not solve any of the naming and hook up to owner
> issues.

Ok. In that case, I've left the RegExp class as the owner.

[Ivan Posva, 2014-11-14 08:42:50]

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:73: CORE_REGEXP_LIB_INTRINSIC_LIST(SETUP_FUNCTION);
Did you add an intrinsic for JSCRE?

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_arm.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1976: static const intptr_t kRegExpParamOffset =
+ 2 * kWordSize;
+ 2 is is unusual style.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1978: // const Smi& start_index is located at  
(+ 0 * kWordSize).
Code in comments?

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1981: // R0: The appropriate specialized matcher
function.
These register assignments are not valid at this point. See line 1767 above for
an example.
In general a comment documenting registers coming in briefly describes the
operation being done in the block followed by a list of current register values.

Incoming we have this following setup:
// R0: Function. (Will be reloaded with the correct target function).
// R4: Arguments descriptor. (Needs to be preserved).
// R5: IC-Data (for methods).

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1999: // IC-Data in R5. Explicitly null out
IC-Data to ensure its validity.
It is not clear to me from this comment why the IC-data needs to be set to 0.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_arm64.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm64.cc:1522: void
Intrinsifier::JSRegExp_ExecuteMatch(Assembler* assembler) {
ditto

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_ia32.cc:2056: void
Intrinsifier::JSRegExp_ExecuteMatch(Assembler* assembler) {
ditto

stub_code_ia32.cc can also serve as a good example
(StubCode::GenerateLazyCompileStub)

https://codereview.chromium.org/683433003/diff/220001/tests/corelib/corelib.s...
File tests/corelib/corelib.status (right):

https://codereview.chromium.org/683433003/diff/220001/tests/corelib/corelib.s...
tests/corelib/corelib.status:225: # JSCRE does not pass all of the tests ported
for irregexp.
Comment is slightly wrong as JSCRE is not going to be a thing.

[zerny-google, 2014-11-14 12:18:57]

Thanks for the comments. Addressed in the latest patch set.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier.cc
File runtime/vm/intrinsifier.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier.cc:73: CORE_REGEXP_LIB_INTRINSIC_LIST(SETUP_FUNCTION);
On 2014/11/14 08:42:49, Ivan Posva wrote:
> Did you add an intrinsic for JSCRE?

Yes. So I missed this because it is not compatible with snapshotting. Slava
explained that the snapshot state can cause the the functions to-be/not-to-be
intrinsified regardless of the flag, so this flag can lead to an inconsistent
state. Instead I've removed the conditional and inserted early escapes in the
intrinsic bodies.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_arm.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1976: static const intptr_t kRegExpParamOffset =
+ 2 * kWordSize;
On 2014/11/14 08:42:50, Ivan Posva wrote:
> + 2 is is unusual style.

Done.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1978: // const Smi& start_index is located at  
(+ 0 * kWordSize).
On 2014/11/14 08:42:50, Ivan Posva wrote:
> Code in comments?

Done.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1981: // R0: The appropriate specialized matcher
function.
On 2014/11/14 08:42:50, Ivan Posva wrote:
> These register assignments are not valid at this point. See line 1767 above
for
> an example.
> In general a comment documenting registers coming in briefly describes the
> operation being done in the block followed by a list of current register
values.
> 
> Incoming we have this following setup:
> // R0: Function. (Will be reloaded with the correct target function).
> // R4: Arguments descriptor. (Needs to be preserved).
> // R5: IC-Data (for methods).

Done.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1999: // IC-Data in R5. Explicitly null out
IC-Data to ensure its validity.
On 2014/11/14 08:42:50, Ivan Posva wrote:
> It is not clear to me from this comment why the IC-data needs to be set to 0.

I think maybe some previous version of the stub used the ic-data register as a
temp and so it was cleared here. Removed the unneeded clearing from all
platforms.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_arm64.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm64.cc:1522: void
Intrinsifier::JSRegExp_ExecuteMatch(Assembler* assembler) {
On 2014/11/14 08:42:50, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/683433003/diff/220001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_ia32.cc:2056: void
Intrinsifier::JSRegExp_ExecuteMatch(Assembler* assembler) {
On 2014/11/14 08:42:50, Ivan Posva wrote:
> ditto
> 
> stub_code_ia32.cc can also serve as a good example
> (StubCode::GenerateLazyCompileStub)

Done.

https://codereview.chromium.org/683433003/diff/220001/tests/corelib/corelib.s...
File tests/corelib/corelib.status (right):

https://codereview.chromium.org/683433003/diff/220001/tests/corelib/corelib.s...
tests/corelib/corelib.status:225: # JSCRE does not pass all of the tests ported
for irregexp.
On 2014/11/14 08:42:50, Ivan Posva wrote:
> Comment is slightly wrong as JSCRE is not going to be a thing.

Rephrased to:

# Ignore the regexp tests on runtimes without irregexp.

as I think dartium would still be using jscre until they roll a new VM, no?

[Ivan Posva, 2014-11-24 06:49:02]

Generally LGTM especially with the follow up patches.

Please work with John to make sure that the Observatory displays some sensible
information for the irregexp based functions.

-Ivan

https://codereview.chromium.org/683433003/diff/220001/tests/corelib/corelib.s...
File tests/corelib/corelib.status (right):

https://codereview.chromium.org/683433003/diff/220001/tests/corelib/corelib.s...
tests/corelib/corelib.status:225: # JSCRE does not pass all of the tests ported
for irregexp.
On 2014/11/14 12:18:57, zerny-google wrote:
> On 2014/11/14 08:42:50, Ivan Posva wrote:
> > Comment is slightly wrong as JSCRE is not going to be a thing.
> 
> Rephrased to:
> 
> # Ignore the regexp tests on runtimes without irregexp.
> 
> as I think dartium would still be using jscre until they roll a new VM, no?

Dartium builds with TOT bleeding edge VMs, so it should have the same capability
as the standalone VM.

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:86: class DefaultCompilationPipeline : public
CompilationPipeline {
Maybe this should be DartCompilationPipeline?

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:110: class IrregexpCompilationPipeline : public
DefaultCompilationPipeline {
Shouldn't this inherit from CompilationPipeline directly? There is nothing to be
inherited from the default Dart compilation pipeline.

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/method_recog...
File runtime/vm/method_recognizer.h (right):

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/method_recog...
runtime/vm/method_recognizer.h:203: #define CORE_REGEXP_LIB_INTRINSIC_LIST(V)   
                                  \
Is this special handling still needed now that the intrinsifier properly deals
with JSCRE being enabled?

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/symbols.h
File runtime/vm/symbols.h (right):

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/symbols.h#ne...
runtime/vm/symbols.h:335: V(stack_, "stack_")                                   
                      \
Generally we name compiler internal variables starting with a : such as
":expr_temp".

[zerny-google, 2014-11-24 13:07:03]

Thanks for the reviews! The observatory looks reasonable (entries are marked by
RegExp.irregexp) and I'll talk with John about possible improvements. Regarding
tests, the tests are not verified to be compatible with jsshell and other
browser backends so I'll leave the non-d8/vm skip in place.

This CL has been merged with the other irregexp CLs into which is ready for
commit: https://codereview.chromium.org/744853003/

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:86: class DefaultCompilationPipeline : public
CompilationPipeline {
On 2014/11/24 06:49:02, Ivan Posva wrote:
> Maybe this should be DartCompilationPipeline?

Done.

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:110: class IrregexpCompilationPipeline : public
DefaultCompilationPipeline {
On 2014/11/24 06:49:02, Ivan Posva wrote:
> Shouldn't this inherit from CompilationPipeline directly? There is nothing to
be
> inherited from the default Dart compilation pipeline.

Done.

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/method_recog...
File runtime/vm/method_recognizer.h (right):

https://codereview.chromium.org/683433003/diff/260001/runtime/vm/method_recog...
runtime/vm/method_recognizer.h:203: #define CORE_REGEXP_LIB_INTRINSIC_LIST(V)   
                                  \
On 2014/11/24 06:49:02, Ivan Posva wrote:
> Is this special handling still needed now that the intrinsifier properly deals
> with JSCRE being enabled?

Not needed any more. Folded it into CORE_LIB_INTRINSIC_LIST.