Port and integrate the irregexp engine from V8

Port and integrate the irregexp engine from V8

This commit contains port of the entire irregexp codebase parsing,
through generation of the internal irregexp IR, to emission of dart VM
IR, and finally function compilation and execution.

Four new IR instructions are introduced:
* IndirectGotoInstr: Jumps to an address specified as its parameter.
* IndirectEntryInstr: The target of an indirect goto.
* CaseInsensitiveCompareUC16Instr: Performs a case-insensitive
  comparison of the specified string regions.
* LoadCodeUnitsInstr: Loads one or more code units from a string.


BUG=

[jgruber1, 2014-09-04 15:04:04]

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

[jgruber1, 2014-09-04 15:04:04]

All tests in our current vm suite pass.

This still produces unoptimized code, so performance is currently in the same
order of magnitude as jscre.

A few porting TODOs still exist, and one or two code paths aren't implemented
yet.

[jgruber1, 2014-09-04 15:05:26]

Correction:

co19/LibTest/core/RegExp/Pattern_semantics/firstMatch_Term_A04_t01

Fails, but we exhibit the same behavior as in V8. Pinged Lasse on what the
correct thing to do is here.

[ricow1, 2014-09-05 05:47:40]

ricow@google.com changed reviewers:
+ ricow@google.com

[ricow1, 2014-09-05 05:47:41]

dbc

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
File tests/language/regex/regexp_test.dart (right):

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:11: void assertEquals(actual, expected,
[message]) => Expect.equals(actual, expected, message);
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:30: re = new RegExp(r"^.", multiLine:
true); // any non-newline character at the beginning of a line
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:40: re = new RegExp(r".$", multiLine:
true); // any non-newline character at the end of a line
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:50: re = new RegExp(r"^[^]", multiLine:
true); // *any* character at the beginning of a line
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:61: re = new RegExp(r"[^]$", multiLine:
true); // *any* character at the end of a line
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:101: re = r"^[\c_]$";  // Underscore
control character is masked in character classes.
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:255: if (doit) return "".replaceAll(new
RegExp(r"foo(", caseSensitive: false), "");
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:264: assertEquals("t",
"123456789t".replaceAllMapped(re, (Match m) => m.group(10)));
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:264: assertEquals("t",
"123456789t".replaceAllMapped(re, (Match m) => m.group(10)));
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:265: assertEquals("15",
"123456789t".replaceAllMapped(re, (Match m) => "${m.group(1)}5"));
long line

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:288: assertEquals("a", new
RegExp(r"x(a)\1x", caseSensitive: false).firstMatch("xaAx").group(1),
"backref-ASCII");
long line + a lot below

https://codereview.chromium.org/539153002/diff/1/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/539153002/diff/1/tools/test.dart#newcode63
tools/test.dart:63: new Path('tests/language/regex'),
You don't need this, we automatically recurse into the subdirectories. You can
validate this by running:
tools/test.py -mrelease -cnone -rvm language/reg -v
and see that your test is run

[ricow1, 2014-09-05 05:49:58]

a little more nit

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

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp.h#newcode1126
runtime/vm/regexp.h:1126: intptr_t max_lookahead,
indentation

[jgruber1, 2014-09-05 06:17:26]

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

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp.h#newcode1126
runtime/vm/regexp.h:1126: intptr_t max_lookahead,
On 2014/09/05 05:49:58, ricow1 wrote:
> indentation 

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
File tests/language/regex/regexp_test.dart (right):

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:11: void assertEquals(actual, expected,
[message]) => Expect.equals(actual, expected, message);
On 2014/09/05 05:47:40, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:30: re = new RegExp(r"^.", multiLine:
true); // any non-newline character at the beginning of a line
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:40: re = new RegExp(r".$", multiLine:
true); // any non-newline character at the end of a line
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:50: re = new RegExp(r"^[^]", multiLine:
true); // *any* character at the beginning of a line
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:61: re = new RegExp(r"[^]$", multiLine:
true); // *any* character at the end of a line
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:101: re = r"^[\c_]$";  // Underscore
control character is masked in character classes.
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:255: if (doit) return "".replaceAll(new
RegExp(r"foo(", caseSensitive: false), "");
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:264: assertEquals("t",
"123456789t".replaceAllMapped(re, (Match m) => m.group(10)));
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:264: assertEquals("t",
"123456789t".replaceAllMapped(re, (Match m) => m.group(10)));
On 2014/09/05 05:47:41, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:265: assertEquals("15",
"123456789t".replaceAllMapped(re, (Match m) => "${m.group(1)}5"));
On 2014/09/05 05:47:40, ricow1 wrote:
> long line

Done.

https://codereview.chromium.org/539153002/diff/1/tests/language/regex/regexp_...
tests/language/regex/regexp_test.dart:288: assertEquals("a", new
RegExp(r"x(a)\1x", caseSensitive: false).firstMatch("xaAx").group(1),
"backref-ASCII");
On 2014/09/05 05:47:41, ricow1 wrote:
> long line + a lot below

Done.

https://codereview.chromium.org/539153002/diff/1/tools/test.dart
File tools/test.dart (right):

https://codereview.chromium.org/539153002/diff/1/tools/test.dart#newcode63
tools/test.dart:63: new Path('tests/language/regex'),
On 2014/09/05 05:47:41, ricow1 wrote:
> You don't need this, we automatically recurse into the subdirectories. You can
> validate this by running:
> tools/test.py -mrelease -cnone -rvm language/reg -v
> and see that your test is run
> 

Good to know, thanks. Done.

[Florian Schneider, 2014-09-16 11:11:49]

First round of comments on the regexp back-end.

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

https://codereview.chromium.org/539153002/diff/1/runtime/lib/regexp.cc#newcode95
runtime/lib/regexp.cc:95: const bool is_one_byte_string = str.IsOneByteString();
Could be also ExternalOneByteString.

https://codereview.chromium.org/539153002/diff/1/runtime/lib/regexp.cc#newcod...
runtime/lib/regexp.cc:149: // Create the output array.
Creating the output array should be part of the generated code so that we can
return without going to C++ first.

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

https://codereview.chromium.org/539153002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:6744: case RawFunction::kIrregexpFunction:
Check for other places where you need to add a case for the new kind.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/539153002/diff/1/runtime/vm/parser.h#newcode147
runtime/vm/parser.h:147: void AllocateIrregexpVariables(intptr_t
num_stack_locals);
Why does AllocateVariables not work?

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler.cc
File runtime/vm/regexp_assembler.cc (right):

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:135: current_character_ =
Local("current_character_");
Make those strings VM symbols, so that they are shared across isolates.

Then you can use static accessors to get a String-Handle. e.g. 

Symbols::CurrentCharacterVar()

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:180: Bind(InstanceCall("get:length",
string_push)));
String::ZoneHandle(Field::GetterName(Symbols::Length())) instead of "get:length"

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:180: Bind(InstanceCall("get:length",
string_push)));
Have you tried directly using LoadField / StoreField instead of getters/setter
calls?

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:197:
String::Handle(Symbols::New("_JSSyntaxRegExp"))));
Just Symbols::JSSyntaxRegExp()

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:201: Symbols::New("_wordCharacterMap"))))));
Add this string to the list of VM symbols in symbols.h and use static accessor
Symbols::xyz() instead of ::New.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:203: StoreLocal(word_character_map_,
Bind(StaticCall(word_character_getter)));
The wordCharacterMap should be embedded as a compile-time constant in the code.
(ConstantInstr)

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:219: Bind(InstanceCall("[]", block_offsets_push,
block_id_push));
s/"[]"/Symbols::IndexToken()/

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:242: Do(InstanceCall("[]=", matches_push,
index_push, value_push));
s/[]=/Symbols::AssignIndexToken()/

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:287: UNREACHABLE();
Since exceptions can happen in generated code (e.g. StackOverflow), so they need
to be propagated further. Look at other uses of InvokeFunction on how to do
this.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:294: intptr_t
IRRegExpMacroAssembler::CaseInsensitiveCompareUC16(
Where is this function used?

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:328: LocalVariable*
IRRegExpMacroAssembler::Parameter(const char* name,
const String& name

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:343: LocalVariable*
IRRegExpMacroAssembler::Local(const char* name) {
const String& name

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:461: const char *name,
const String& name

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:472: const char *name,
const String& name

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:485: const char *name,
const String& name

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:500: const char *name,
const String& name

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:507: String::ZoneHandle(I, Symbols::New(name)),
Just use name.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:762: void
IRRegExpMacroAssembler::CheckCharacter(uint32_t c, BlockLabel* on_equal) {
Isn't characters uint16_t as most?

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:796: InstanceCall("unary-",
If not present, add unary- to VM symbols in symbols.h and use static accessor.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:818: InstanceCall("unary-",
If not present, add unary- to VM symbols in symbols.h and use static accessor.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:1750:
String::Handle(Symbols::New("_codeUnitsAt"))).ToCString();
Add _codeUnitsAt to VM symbols and use static accessor. - Same for other
instance calls.

[jgruber1, 2014-09-22 18:58:06]

The initial CL has been completely overhauled:

* We now use the optimizing compiler pipeline.
* JSSyntaxRegExp::_ExecuteMatch is intrinsified.
* String::_codeUnitsAt methods are optimized to a new instruction, similar to an
unchecked and LoadIndexedInstr that can load more than one element.
* IndirectGotoInstr/IndirectEntryInstr are now similar to a multiway branch,
enabling all optimizations that were previously impossible when indirect gotos
were treated like catch/throw.
* Remaining code paths, most notably case-insensitive comparison of unicode
strings, have been ported.
* All architectures except MIPS should be up and running (MIPS currently falls
back to JSCRE, but should follow soon).
* And much more.

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

https://codereview.chromium.org/539153002/diff/1/runtime/lib/regexp.cc#newcode95
runtime/lib/regexp.cc:95: const bool is_one_byte_string = str.IsOneByteString();
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Could be also ExternalOneByteString.

The generated code is now specialized for all four string types.

https://codereview.chromium.org/539153002/diff/1/runtime/lib/regexp.cc#newcod...
runtime/lib/regexp.cc:149: // Create the output array.
On 2014/09/16 11:11:47, Florian Schneider wrote:
> Creating the output array should be part of the generated code so that we can
> return without going to C++ first.

Done.

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

https://codereview.chromium.org/539153002/diff/1/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:6744: case RawFunction::kIrregexpFunction:
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Check for other places where you need to add a case for the new kind. 

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/parser.h
File runtime/vm/parser.h (right):

https://codereview.chromium.org/539153002/diff/1/runtime/vm/parser.h#newcode147
runtime/vm/parser.h:147: void AllocateIrregexpVariables(intptr_t
num_stack_locals);
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Why does AllocateVariables not work?

AllocateVariables calculates the number of locals, while it is passed into
AllocateIrregexpVariables as an argument (ultimately it is taken from the regexp
macro assembler after compilation to Dart IR). 

The logic of the function itself is short enough that it seemed like a better
choice to create a specialized version just for irregexps.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler.cc
File runtime/vm/regexp_assembler.cc (right):

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:135: current_character_ =
Local("current_character_");
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Make those strings VM symbols, so that they are shared across isolates.
> 
> Then you can use static accessors to get a String-Handle. e.g. 
> 
> Symbols::CurrentCharacterVar()

Done. Added variable names, function names, etc. to the symbols list.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:180: Bind(InstanceCall("get:length",
string_push)));
On 2014/09/16 11:11:48, Florian Schneider wrote:
> String::ZoneHandle(Field::GetterName(Symbols::Length())) instead of
"get:length"

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:180: Bind(InstanceCall("get:length",
string_push)));
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Have you tried directly using LoadField / StoreField instead of getters/setter
> calls?

No, since optimization has the same effect and the amount of time spent in the
unoptimized version has been insignificant in all benchmarks I tried.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:197:
String::Handle(Symbols::New("_JSSyntaxRegExp"))));
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Just Symbols::JSSyntaxRegExp()

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:201: Symbols::New("_wordCharacterMap"))))));
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Add this string to the list of VM symbols in symbols.h and use static accessor
> Symbols::xyz() instead of ::New.

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:203: StoreLocal(word_character_map_,
Bind(StaticCall(word_character_getter)));
On 2014/09/16 11:11:48, Florian Schneider wrote:
> The wordCharacterMap should be embedded as a compile-time constant in the
code.
> (ConstantInstr)

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:219: Bind(InstanceCall("[]", block_offsets_push,
block_id_push));
On 2014/09/16 11:11:49, Florian Schneider wrote:
> s/"[]"/Symbols::IndexToken()/

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:242: Do(InstanceCall("[]=", matches_push,
index_push, value_push));
On 2014/09/16 11:11:49, Florian Schneider wrote:
> s/[]=/Symbols::AssignIndexToken()/

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:287: UNREACHABLE();
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Since exceptions can happen in generated code (e.g. StackOverflow), so they
need
> to be propagated further. Look at other uses of InvokeFunction on how to do
> this.

The entry into irregexp matcher functions is now an intrinsic.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:294: intptr_t
IRRegExpMacroAssembler::CaseInsensitiveCompareUC16(
On 2014/09/16 11:11:48, Florian Schneider wrote:
> Where is this function used?

This was one of the last unported bits. It is now called from a new
CaseInsensitiveCompareUC16Instr.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:328: LocalVariable*
IRRegExpMacroAssembler::Parameter(const char* name,
On 2014/09/16 11:11:48, Florian Schneider wrote:
> const String& name

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:343: LocalVariable*
IRRegExpMacroAssembler::Local(const char* name) {
On 2014/09/16 11:11:48, Florian Schneider wrote:
> const String& name

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:461: const char *name,
On 2014/09/16 11:11:49, Florian Schneider wrote:
> const String& name

These have been refactored to handle # of checked arguments and token kinds for
operators & friends.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:472: const char *name,
On 2014/09/16 11:11:48, Florian Schneider wrote:
> const String& name

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:485: const char *name,
On 2014/09/16 11:11:48, Florian Schneider wrote:
> const String& name

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:500: const char *name,
On 2014/09/16 11:11:49, Florian Schneider wrote:
> const String& name

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:507: String::ZoneHandle(I, Symbols::New(name)),
On 2014/09/16 11:11:49, Florian Schneider wrote:
> Just use name.

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:762: void
IRRegExpMacroAssembler::CheckCharacter(uint32_t c, BlockLabel* on_equal) {
On 2014/09/16 11:11:49, Florian Schneider wrote:
> Isn't characters uint16_t as most?

No, at present up to 4 ASCII or 2 UTF16 code units may be packed into a
uint32_t.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:796: InstanceCall("unary-",
On 2014/09/16 11:11:48, Florian Schneider wrote:
> If not present, add unary- to VM symbols in symbols.h and use static accessor.

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:818: InstanceCall("unary-",
On 2014/09/16 11:11:48, Florian Schneider wrote:
> If not present, add unary- to VM symbols in symbols.h and use static accessor.

Done.

https://codereview.chromium.org/539153002/diff/1/runtime/vm/regexp_assembler....
runtime/vm/regexp_assembler.cc:1750:
String::Handle(Symbols::New("_codeUnitsAt"))).ToCString();
On 2014/09/16 11:11:49, Florian Schneider wrote:
> Add _codeUnitsAt to VM symbols and use static accessor. - Same for other
> instance calls.

Done.

[srdjan, 2014-09-22 22:34:06]

srdjan@google.com changed reviewers:
+ srdjan@google.com

[srdjan, 2014-09-22 22:34:08]

DBC

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

https://codereview.chromium.org/539153002/diff/20001/runtime/lib/regexp.cc#ne...
runtime/lib/regexp.cc:99: // This function is intrinsified. See
Intrinsifier::JSRegExp_ExecuteMatch.
We occasionally run with --no-intrinsify. Will it work then?

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/flow_graph_ty...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:725: default: UNREACHABLE();
Indent case (see other switch examples).

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1139: ASSERT(index < catch_entries_.length()
+ 1);
Add parentheses.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:2186: Value* offset_from_start)
remove explicit

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:2201: void adjust_edge_weight(double
scale_factor) { edge_weight_ *= scale_factor; }
s/adjust_edge_weight/scale_edge_weight/ to make it more readable at call site?

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intrinsifier_...
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intrinsifier_...
runtime/vm/intrinsifier_ia32.cc:1784: 
Registers EDX (arguments descriptor) and ECX (function) must be normally
preserved. Please explain here why it is OK not do so.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/regexp_assemb...
File runtime/vm/regexp_assembler.h (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/regexp_assemb...
runtime/vm/regexp_assembler.h:62: class RegExpMacroAssembler {
Is this object allocated in the C-heap?

[jgruber1, 2014-09-23 10:58:47]

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

https://codereview.chromium.org/539153002/diff/20001/runtime/lib/regexp.cc#ne...
runtime/lib/regexp.cc:99: // This function is intrinsified. See
Intrinsifier::JSRegExp_ExecuteMatch.
On 2014/09/22 22:34:07, srdjan wrote:
> We occasionally run with --no-intrinsify. Will it work then?

Good point, I wasn't aware of that flag. Added back the code for the
non-intrinsified version of this function.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/flow_graph_ty...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/flow_graph_ty...
runtime/vm/flow_graph_type_propagator.cc:725: default: UNREACHABLE();
On 2014/09/22 22:34:07, srdjan wrote:
> Indent case (see other switch examples).

Done.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.cc:1139: ASSERT(index < catch_entries_.length()
+ 1);
On 2014/09/22 22:34:07, srdjan wrote:
> Add parentheses.

Reverted this change since it was functionally identical to its its previous
state.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:2186: Value* offset_from_start)
On 2014/09/22 22:34:07, srdjan wrote:
> remove explicit

Done.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intermediate_...
runtime/vm/intermediate_language.h:2201: void adjust_edge_weight(double
scale_factor) { edge_weight_ *= scale_factor; }
On 2014/09/22 22:34:07, srdjan wrote:
> s/adjust_edge_weight/scale_edge_weight/ to make it more readable at call site?

edge_weight_ was left over from duplicating GotoInstr, removed it & associated
functions.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intrinsifier_...
File runtime/vm/intrinsifier_ia32.cc (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/intrinsifier_...
runtime/vm/intrinsifier_ia32.cc:1784: 
On 2014/09/22 22:34:07, srdjan wrote:
> Registers EDX (arguments descriptor) and ECX (function) must be normally
> preserved. Please explain here why it is OK not do so.

Done.

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/regexp_assemb...
File runtime/vm/regexp_assembler.h (right):

https://codereview.chromium.org/539153002/diff/20001/runtime/vm/regexp_assemb...
runtime/vm/regexp_assembler.h:62: class RegExpMacroAssembler {
On 2014/09/22 22:34:07, srdjan wrote:
> Is this object allocated in the C-heap?

Changed inheritance hierarchy to:

ZoneAllocated <- RegExpMacroAssembler <- IRRegExpMacroAssembler

instead of

RegExpMacroAssembler <- IRRegExpMacroAssembler
       ZoneAllocated <-/

[Florian Schneider, 2014-10-01 17:04:15]

Here are more comments. I'm partially through regexp_assembler.cc, and will
continue tomorrow.

Overall I think it looks already very good! Let's go through few the non-trivial
comments in person tomorrow as well.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:101: Function& fn =
Function::Handle(regexp.function(cid));
const Function&

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:104: if (fn.CurrentCode() ==
StubCode::LazyCompile_entry()->code()) {
In the instrinc assembly code you check

(sample_subject == null)

here. It should be the equivalent (fn is compiled on first invocation) - but for
consistency I'd make it the same (i.e. use the same condition here.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:810: if (flow_graph().IsIrregexpFunction()) {
if (flow_graph().IsIrregexpFunction()) return;

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1048: !function.IsIrregexpFunction();
Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't it?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm64.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm64.cc:1052: !function.IsIrregexpFunction();
Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't it?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1047: !function.IsIrregexpFunction();
Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't it?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_mips.cc:1078: !function.IsIrregexpFunction();
Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't it?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_x64.cc:1085: !function.IsIrregexpFunction();
Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't it?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:8875: CaseInsensitiveCompareUC16Instr *instr)
{
Not sure if it's actually worth supporting: do you see cases where all inputs
are constants?

Maybe just simplify the code and say

SetValue(instr, non_constant_);

instead.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.cc:1995:
RangeBoundary::FromConstant(0xFFFFFFFF));
s/0xFFFFFFFF/kMaxUint32/

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_t...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_t...
runtime/vm/flow_graph_type_propagator.cc:1308: return
CompileType::FromCid(kSmiCid);
The return type is Bool, not Smi.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:378: element_count() ==
other_load->element_count());
Did you forget checking index_scale()?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1789: Definition*
CaseInsensitiveCompareUC16Instr::Canonicalize(
Just remove CaseInsensitiveCompareUC16Instr::Canonicalize in this case.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:893: friend class
CaseInsensitiveCompareUC16Instr;
Not needed?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:914: friend class IndirectEntryInstr;
Not needed?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2200: IndirectGotoInstr(GrowableObjectArray*
offsets,
Can this be NULL? If not, just pass a const GrowableObjectArray& instead.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:5196: deopt_id_ = deopt_id;
Not needed, since it can't deoptimize and also can't become deoptimization
target.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1368: return
CompileType::FromCid(element_count() < 4 ? kSmiCid : kMintCid);
kMintCid may not be true: when loading 4 elements the result can be either smi
or mint.

Just use CompileType::Int() in this case.

Also, you can move this function to flow_graph_type_propagator.cc and use
kSmiBits to test if the platform has 31 or 63 bit smis.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1371: return
CompileType::FromCid(element_count() < 2 ? kSmiCid : kMintCid);
Same here.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1383: return element_count() < 4 ?
kTagged : kUnboxedMint;
kUnboxedMint should work. kUnboxedUint32 may be better, but we have to check the
generated code if everything is supported for using uint32.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1402: if (CanBeImmediateIndex(index(),
class_id(), IsExternal(),
I think the code should be simplified by not supporting immediate index:

Let's check if we actually ever pass a constant as index. If not, I'd remove the
code for constant index here.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:4818: summary->set_temp(0,
Location::RegisterLocation(EDI));
At calls, no need to specify temps: All registers are destroyed at calls anyway
and live ones will be saved by the register allocator inserting spill-moves.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:6721: __ SubImmediate(sum_reg,
Immediate(Assembler::EntryPointToPcMarkerOffset()));
Maybe fold this into the add below using lea:

__ leal(sum_reg, Address(offset_reg, -Assembler::EntryPointToPcMarkerOffset()));

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:1328: case 4:  // Loading multiple code
units is disabled on MIPS.
Maybe add a TODO for supporting this?

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:5890: __ SubImmediate(sum_reg,
Same as on ia32. Try fold it into the addq below.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_ia32.cc:1786: // IC-Data in ECX. Irregexp generated
functions do not use either the
ECX is destroyed by GenerateRegexpSpecializationFor - it should not matter, as
long as there is a GC-safe value in there. 

Object::null_object() will work, but please update the comment. The IC data are
only required when calling the InlineCacheStub.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20051: case kOneByteStringCid: dest =
&raw_ptr()->one_byte_function_; break;
break lines after if there is more than one statement in a case-statement:

case kOneByteStringCid:
  dest = &raw_ptr()->one_byte_function_;
  break;

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20068: case kOneByteStringCid: dest =
&raw_ptr()->one_byte_subject_; break;
Also here.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/object.h#new...
runtime/vm/object.h:1650: raw_ptr()->regexp_specialization_ = cid;
I think you can use the kind-specific data_ field for storing the
specialization-cid.


Maybe add _cid to the name to make it clearer what it is.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/object.h#new...
runtime/vm/object.h:3527: V(CodeUnitsLoad)                                      
                    \
No need, since I don't see any deoptimizations in LoadCodeUnitsAtInstr.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:661: RawJSRegExp* regexp_;  // The corresponding regexp
for irregexp functions.
Maybe re-use the owner field for this? It is not used otherwise, since those
functions are not member methods of a specific class.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:689: intptr_t regexp_specialization_;
As mentioned, try using the data_ field for that. It is a kind-specific field
that can be used for different purposes depending on function kind.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:2471: TypedData& ba = TypedData::ZoneHandle(
const TypedData&

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4359: TypedData* boolean_skip_table) {
const TypedData& since this will never be NULL.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4510: TypedData& boolean_skip_table =
TypedData::ZoneHandle(
const TypedData&

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:218: StoreLocal(result_,
Bind(StaticCall(list_ctor, type_arg_push, length_push)));
I think it's fine to use the generic List constructor and []= operators for now
- but we could try using CreateArrayInstr and StoreIndexed instead for
performance. I'd add a TODO for now.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:304: const String& str = String::Handle(str_raw);
Is this performance-critical? If not, I'd make it a normal native method like
other natives. If yes, this won't be particularily fast because of handle
allocation - if the type of the input string is known, then it can be
specialized for each String type.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:312: // This function is not allowed to cause a
garbage collection.
Add a NoGCScope to this function to assert that no GC happens.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:747: ASSERT(index < 10 * 10 * 10 * 10 * 10);  //
10^5.
Where doesthis limit (10^5) come from? We should make it a named constant.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.h (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:22: BlockLabel(const BlockLabel& that)
Is the copy constructor and operator= needed? If yes, I prefer to use the
default-implementation provided by the compiler, instead of having to keep the
implementation in sync with the class declaration manually.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:367: struct InstanceCallDescriptor : public
ValueObject {
Maybe not inherit from ValueObject

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:431: InstanceCallDescriptor(const
InstanceCallDescriptor& that)
Needed?

[Vyacheslav Egorov (Google), 2014-10-01 20:13:22]

initial comments

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

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:107: #ifdef DEBUG
Empty #ifdef ?

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp_patc...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:37: return value.regexp;
Please add TODO here that we might not want to "cannonicalize" regexp objects
themselves like that.

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:65: return (pattern == that.pattern) &&
this.pattern for consistency, or remove this. everywhere in this method

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:2859: LoadCodeUnitsInstr* load = new(I)
LoadCodeUnitsInstr(
[ignore this comment]

This is *a*whole*lot* of code (and also "dead" stuff like that TargetEntryInstr
down there) to produce a single instruction.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/il_printer.c...
runtime/vm/il_printer.cc:585: str()->PrintTo(f);
Isn't this what Definition::PrintOperandsTo(f) does by default? I think you
don't need to override this method.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/il_printer.c...
runtime/vm/il_printer.cc:950: if (try_index() !=
CatchClauseNode::kInvalidTryIndex) {
Can IndirectEntry be inside of the try? Maybe just assert that it can't.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2204: inputs_[0] = offset_from_start_;
please add a comment explaining why you can't SetInputAt(0, ...) here.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:3904: deopt_id_ = deopt_id;
I don't think it can deoptimize.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:7059: Register sum_reg =
locs()->temp_slot(0)->reg();
better call it target_address_reg

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:7070: __ add(sum_reg, sum_reg,
Operand(offset_reg));
you can do

__ add(sum_reg, sum_reg, Operand(offset_reg, ASR, kSmiTagSize));

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_x64.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:1534: __ StoreIntoObject(RBX,
I really don't think there is any good reason to keep sample subject alive after
we compiled compiled regexp - which means you should really null it after regexp
is compiled.

Otherwise there will be hard to debug memory leaks in the system.

There is a lot of complexity in the system because we are trying to make normal
lazy compilation work for us.

I suggest we just do 

mov target, [RBX + JSRegExp::function_offset(cid)];
cmp target, #null
jne call-target
;; compile target explicitly passing subject as an argument

calltarget:
;; target is created and compiled here

This would allow us to kill a lot of fields that you added just to make current
lazy compilation system to work.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:661: RawJSRegExp* regexp_;  // The corresponding regexp
for irregexp functions.
Kill this field by being more explicit about compilation instead of using lazy
compilation stub for this.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:689: intptr_t regexp_specialization_;
Ditto.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1786: RawString* one_byte_subject_;
This four subject fields smell like memory leak to me. Please kill them with
fire.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.h#new...
runtime/vm/regexp.h:1389: explicit CompilationResult(IRRegExpMacroAssembler*
macro_assembler,
No need to mark multiple arguments constructor "explicit"

[Florian Schneider, 2014-10-02 11:58:12]

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2248: Value* offset_from_start_;
Remove this member. SetInputAt(0,...) already stores it.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:6721: __ SubImmediate(sum_reg,
Immediate(Assembler::EntryPointToPcMarkerOffset()));
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Maybe fold this into the add below using lea:
> 
> __ leal(sum_reg, Address(offset_reg,
-Assembler::EntryPointToPcMarkerOffset()));

Even better: subtract the offset Assembler::EntryPointToPcMarkerOffset() at
compile-time to remove this instructions.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.h (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:76: kParamCount = 2
Remove =2 here.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:84: kNoStackLimitCheck = false,
Make this 0.

Or make two static const bool variables.

[jgruber1, 2014-10-03 18:59:54]

Squashed all irregexp commits as requested, addressed comments, PTAL.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:101: Function& fn =
Function::Handle(regexp.function(cid));
On 2014/10/01 17:04:15, Florian Schneider wrote:
> const Function&

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:104: if (fn.CurrentCode() ==
StubCode::LazyCompile_entry()->code()) {
On 2014/10/01 17:04:14, Florian Schneider wrote:
> In the instrinc assembly code you check
> 
> (sample_subject == null)
> 
> here. It should be the equivalent (fn is compiled on first invocation) - but
for
> consistency I'd make it the same (i.e. use the same condition here.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:107: #ifdef DEBUG
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> Empty #ifdef ?

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp_patc...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:37: return value.regexp;
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> Please add TODO here that we might not want to "cannonicalize" regexp objects
> themselves like that.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:65: return (pattern == that.pattern) &&
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> this.pattern for consistency, or remove this. everywhere in this method

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:810: if (flow_graph().IsIrregexpFunction()) {
On 2014/10/01 17:04:14, Florian Schneider wrote:
> if (flow_graph().IsIrregexpFunction()) return;

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm.cc:1048: !function.IsIrregexpFunction();
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't
it?

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_arm64.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_arm64.cc:1052: !function.IsIrregexpFunction();
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't
it?

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_ia32.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_ia32.cc:1047: !function.IsIrregexpFunction();
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't
it?

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_mips.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_mips.cc:1078: !function.IsIrregexpFunction();
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't
it?

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler_x64.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler_x64.cc:1085: !function.IsIrregexpFunction();
On 2014/10/01 17:04:15, Florian Schneider wrote:
> Is this necessary? IsClosureFunction implies !IsIrregexpFunction(), doesn't
it?

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:2859: LoadCodeUnitsInstr* load = new(I)
LoadCodeUnitsInstr(
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> [ignore this comment]
> 
> This is *a*whole*lot* of code (and also "dead" stuff like that
TargetEntryInstr
> down there) to produce a single instruction.
> 

I agree, the reason behind this was that LoadIndexedInstr was not expected in
unoptimized code and we wanted to optimize *codeUnitsAt 'properly'.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:8875: CaseInsensitiveCompareUC16Instr *instr)
{
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Not sure if it's actually worth supporting: do you see cases where all inputs
> are constants?
> 
> Maybe just simplify the code and say
> 
> SetValue(instr, non_constant_);
> 
> instead.

Done.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_r...
runtime/vm/flow_graph_range_analysis.cc:1995:
RangeBoundary::FromConstant(0xFFFFFFFF));
On 2014/10/01 17:04:15, Florian Schneider wrote:
> s/0xFFFFFFFF/kMaxUint32/

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_t...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/flow_graph_t...
runtime/vm/flow_graph_type_propagator.cc:1308: return
CompileType::FromCid(kSmiCid);
On 2014/10/01 17:04:14, Florian Schneider wrote:
> The return type is Bool, not Smi.

Done.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/il_printer.c...
runtime/vm/il_printer.cc:585: str()->PrintTo(f);
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> Isn't this what Definition::PrintOperandsTo(f) does by default? I think you
> don't need to override this method.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/il_printer.c...
runtime/vm/il_printer.cc:950: if (try_index() !=
CatchClauseNode::kInvalidTryIndex) {
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> Can IndirectEntry be inside of the try? Maybe just assert that it can't.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:378: element_count() ==
other_load->element_count());
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Did you forget checking index_scale()?

I removed the index_scale_ member since index_scale is determined by the
class_id.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.cc:1789: Definition*
CaseInsensitiveCompareUC16Instr::Canonicalize(
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Just remove CaseInsensitiveCompareUC16Instr::Canonicalize in this case.

Done.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:893: friend class
CaseInsensitiveCompareUC16Instr;
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Not needed?

Done, unneeded now that deopt_id_ is not accessed in CICUC16Instr.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:914: friend class IndirectEntryInstr;
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Not needed?

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2200: IndirectGotoInstr(GrowableObjectArray*
offsets,
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Can this be NULL? If not, just pass a const GrowableObjectArray& instead.

No, but it is modified in SetOffsetCount() and SetOffsetAt(), and lint does not
like non-const reference parameters.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2204: inputs_[0] = offset_from_start_;
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> please add a comment explaining why you can't SetInputAt(0, ...) here.

Done, changed to SetInputAt().

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2248: Value* offset_from_start_;
On 2014/10/02 11:58:12, Florian Schneider wrote:
> Remove this member. SetInputAt(0,...) already stores it.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:3904: deopt_id_ = deopt_id;
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> I don't think it can deoptimize.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:5196: deopt_id_ = deopt_id;
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Not needed, since it can't deoptimize and also can't become deoptimization
> target.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1368: return
CompileType::FromCid(element_count() < 4 ? kSmiCid : kMintCid);
On 2014/10/01 17:04:15, Florian Schneider wrote:
> kMintCid may not be true: when loading 4 elements the result can be either smi
> or mint.
> 
> Just use CompileType::Int() in this case.
> 
> Also, you can move this function to flow_graph_type_propagator.cc and use
> kSmiBits to test if the platform has 31 or 63 bit smis.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1371: return
CompileType::FromCid(element_count() < 2 ? kSmiCid : kMintCid);
On 2014/10/01 17:04:15, Florian Schneider wrote:
> Same here.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1383: return element_count() < 4 ?
kTagged : kUnboxedMint;
On 2014/10/01 17:04:15, Florian Schneider wrote:
> kUnboxedMint should work. kUnboxedUint32 may be better, but we have to check
the
> generated code if everything is supported for using uint32.

Added a TODO.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1402: if (CanBeImmediateIndex(index(),
class_id(), IsExternal(),
On 2014/10/01 17:04:15, Florian Schneider wrote:
> I think the code should be simplified by not supporting immediate index:
> 
> Let's check if we actually ever pass a constant as index. If not, I'd remove
the
> code for constant index here.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:7059: Register sum_reg =
locs()->temp_slot(0)->reg();
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> better call it target_address_reg

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:7070: __ add(sum_reg, sum_reg,
Operand(offset_reg));
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> you can do
> 
> __ add(sum_reg, sum_reg, Operand(offset_reg, ASR, kSmiTagSize));

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:4818: summary->set_temp(0,
Location::RegisterLocation(EDI));
On 2014/10/01 17:04:14, Florian Schneider wrote:
> At calls, no need to specify temps: All registers are destroyed at calls
anyway
> and live ones will be saved by the register allocator inserting spill-moves.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:6721: __ SubImmediate(sum_reg,
Immediate(Assembler::EntryPointToPcMarkerOffset()));
On 2014/10/02 11:58:12, Florian Schneider wrote:
> On 2014/10/01 17:04:14, Florian Schneider wrote:
> > Maybe fold this into the add below using lea:
> > 
> > __ leal(sum_reg, Address(offset_reg,
> -Assembler::EntryPointToPcMarkerOffset()));
> 
> Even better: subtract the offset Assembler::EntryPointToPcMarkerOffset() at
> compile-time to remove this instructions.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_ia32.cc:6721: __ SubImmediate(sum_reg,
Immediate(Assembler::EntryPointToPcMarkerOffset()));
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Maybe fold this into the add below using lea:
> 
> __ leal(sum_reg, Address(offset_reg,
-Assembler::EntryPointToPcMarkerOffset()));

Acknowledged.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_mips.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/intermediate...
runtime/vm/intermediate_language_mips.cc:1328: case 4:  // Loading multiple code
units is disabled on MIPS.
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Maybe add a TODO for supporting this?

The associated TODO is for kEnableUnalignedAccesses in regexp_assembler.cc.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/object.h#new...
runtime/vm/object.h:1650: raw_ptr()->regexp_specialization_ = cid;
On 2014/10/01 17:04:15, Florian Schneider wrote:
> I think you can use the kind-specific data_ field for storing the
> specialization-cid.
> 
> 
> Maybe add _cid to the name to make it clearer what it is.

data_ now stores the RawJSRegExp pointer, and I renamed this to regexp_cid_.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/object.h#new...
runtime/vm/object.h:3527: V(CodeUnitsLoad)                                      
                    \
On 2014/10/01 17:04:15, Florian Schneider wrote:
> No need, since I don't see any deoptimizations in LoadCodeUnitsAtInstr.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:661: RawJSRegExp* regexp_;  // The corresponding regexp
for irregexp functions.
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> Kill this field by being more explicit about compilation instead of using lazy
> compilation stub for this.

As discussed, the regexp object pointer is now stored in data_.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:661: RawJSRegExp* regexp_;  // The corresponding regexp
for irregexp functions.
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Maybe re-use the owner field for this? It is not used otherwise, since those
> functions are not member methods of a specific class.

As discussed, the regexp object pointer is now stored in data_.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:689: intptr_t regexp_specialization_;
On 2014/10/01 17:04:14, Florian Schneider wrote:
> As mentioned, try using the data_ field for that. It is a kind-specific field
> that can be used for different purposes depending on function kind.

As discussed, changed to int16_t and moved 3 lines up to avoid increasing the
size of RawFunction.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:689: intptr_t regexp_specialization_;
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> Ditto.

As discussed, changed to int16_t and moved 3 lines up to avoid increasing the
size of RawFunction.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1786: RawString* one_byte_subject_;
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> This four subject fields smell like memory leak to me. Please kill them with
> fire.

Done.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:2471: TypedData& ba = TypedData::ZoneHandle(
On 2014/10/01 17:04:14, Florian Schneider wrote:
> const TypedData&

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4359: TypedData* boolean_skip_table) {
On 2014/10/01 17:04:14, Florian Schneider wrote:
> const TypedData& since this will never be NULL.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4510: TypedData& boolean_skip_table =
TypedData::ZoneHandle(
On 2014/10/01 17:04:14, Florian Schneider wrote:
> const TypedData&

Done.

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

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp.h#new...
runtime/vm/regexp.h:1389: explicit CompilationResult(IRRegExpMacroAssembler*
macro_assembler,
On 2014/10/01 20:13:21, Vyacheslav Egorov (Google) wrote:
> No need to mark multiple arguments constructor "explicit"

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.cc (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:218: StoreLocal(result_,
Bind(StaticCall(list_ctor, type_arg_push, length_push)));
On 2014/10/01 17:04:14, Florian Schneider wrote:
> I think it's fine to use the generic List constructor and []= operators for
now
> - but we could try using CreateArrayInstr and StoreIndexed instead for
> performance. I'd add a TODO for now.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:304: const String& str = String::Handle(str_raw);
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Is this performance-critical? If not, I'd make it a normal native method like
> other natives. If yes, this won't be particularily fast because of handle
> allocation - if the type of the input string is known, then it can be
> specialized for each String type.

Acknowledged.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:312: // This function is not allowed to cause a
garbage collection.
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Add a NoGCScope to this function to assert that no GC happens.

Removing comment as discussed in person since code cannot be moved during GC in
Dart.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:747: ASSERT(index < 10 * 10 * 10 * 10 * 10);  //
10^5.
On 2014/10/01 17:04:14, Florian Schneider wrote:
> Where doesthis limit (10^5) come from? We should make it a named constant.

I removed all of the name generation code since it was mostly used for
debugging. Position register locals will most likely be removed in the writing
in favor of accessing the results `matches` parameter list directly.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.h (right):

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:22: BlockLabel(const BlockLabel& that)
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Is the copy constructor and operator= needed? If yes, I prefer to use the
> default-implementation provided by the compiler, instead of having to keep the
> implementation in sync with the class declaration manually.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:76: kParamCount = 2
On 2014/10/02 11:58:12, Florian Schneider wrote:
> Remove =2 here.

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:84: kNoStackLimitCheck = false,
On 2014/10/02 11:58:12, Florian Schneider wrote:
> Make this 0.
> 
> Or make two static const bool variables.

Removed the entire enum.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:367: struct InstanceCallDescriptor : public
ValueObject {
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Maybe not inherit from ValueObject

Done.

https://codereview.chromium.org/539153002/diff/100001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:431: InstanceCallDescriptor(const
InstanceCallDescriptor& that)
On 2014/10/01 17:04:13, Florian Schneider wrote:
> Needed?

Done.

[zerny-google, 2014-10-07 12:18:33]

zerny@google.com changed reviewers:
+ zerny@google.com

[zerny-google, 2014-10-07 12:18:35]

This looks really good! Inlined are a few nits/comments.

https://codereview.chromium.org/539153002/diff/110001/runtime/lib/regexp_patc...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/539153002/diff/110001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:50: static final
LinkedList<_JSSyntaxRegExpHashKey> _leastRecentlyUsed =
Nit: Maybe this should just be named _recentlyUsed since it is ordered from
most-recent to least-recent.

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/flow_graph_t...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/flow_graph_t...
runtime/vm/flow_graph_type_propagator.cc:1030: CompileType
LoadCodeUnitsInstr::ComputeType() const {
Maybe just remove the "fast path" and make the relationship in each case more
clear by using:

case kOneByteStringCid:
case kExternalOneByteStringCid:
  ASSERT(element_count() <= 4);
  return (element_count() <= kSmiBits/8) ? ...;

Also, since the code is assuming that element_count is one of 1, 2 or 4 I'd
suggest we have an enum/const encoding of code units using kOneCodeUnit,
kTwoCodeUnit and kFourCodeUnit making it clear that these are the only values.

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

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:3915: // version of LoadIndexedInstr which
accepts only string targets may load
which accepts only string targets may load
->
which accepts only string targets and can load

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1438: case 2: __ ldrh(result,
element_address); break;
Nit: the fact that case 3 is not missing would be a bit more clear with the
explicit type of code units.

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6993: LocationSummary* locs =
new(isolate) LocationSummary(
Nit: Not sure if locs or summary is the preferred naming convention. I'd stick
with just one for your changes...

[Florian Schneider, 2014-10-07 12:49:37]

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm64.cc:1540: __ LoadClassId(R1, R1, PP);
I don't think you can use PP here, since the pool register is not set up like it
is in normal function here in intrinsic code. The FlowGraphCompiler disables the
constant pool for instrinsics, but better use kNoPP instead to be safe.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm64.cc:1541: __ AddImmediate(R1, R1,
-kOneByteStringCid, PP);
kNoPP here and below as well.

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_ia32.cc:2095: __ movl(ECX,
Immediate(reinterpret_cast<intptr_t>(Object::null())));
Alternatively, just store smi 0:

__ xorl(ECX, ECX);

(applies to the other architetures as well)

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.cc (right):

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:396: Array::Cast(Instance::ZoneHandle(I,
word_character_field.value())));
No need for Array::Cast here.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:940:
InstanceCall(InstanceCallDescriptor::FromToken(Token::kADD),
Use helper IRRegExpMacroAssembler::Add here?

Also: Some helpers perform Bind/Do, others like ::InstanceCall don't.

I'd make them consistent. Maybe never perform Bind/Do inside these, so there is
less danger of using them where evaluation order could be wrong.

You'd need to change ::Add, ::Sub, etc. to match what ::Comparison does: Take
Definition* as argument and Bind them inside.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:1760: return Bind(InstanceCall(
I'd rather not perform Bind() here, and just return Definition*

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:1768: return Bind(InstanceCall(
Also here better not to call Bind() inside.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.h (right):

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:610: IdAllocator block_id;
Rename private members to have _ at the end

block_id_
temp_id_
etc...

[jgruber1, 2014-10-07 15:00:25]

https://codereview.chromium.org/539153002/diff/110001/runtime/lib/regexp_patc...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/539153002/diff/110001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:50: static final
LinkedList<_JSSyntaxRegExpHashKey> _leastRecentlyUsed =
On 2014/10/07 12:18:35, zerny-google wrote:
> Nit: Maybe this should just be named _recentlyUsed since it is ordered from
> most-recent to least-recent.

Done.

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/flow_graph_t...
File runtime/vm/flow_graph_type_propagator.cc (right):

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/flow_graph_t...
runtime/vm/flow_graph_type_propagator.cc:1030: CompileType
LoadCodeUnitsInstr::ComputeType() const {
On 2014/10/07 12:18:35, zerny-google wrote:
> Maybe just remove the "fast path" and make the relationship in each case more
> clear by using:
> 
> case kOneByteStringCid:
> case kExternalOneByteStringCid:
>   ASSERT(element_count() <= 4);
>   return (element_count() <= kSmiBits/8) ? ...;
> 
> Also, since the code is assuming that element_count is one of 1, 2 or 4 I'd
> suggest we have an enum/const encoding of code units using kOneCodeUnit,
> kTwoCodeUnit and kFourCodeUnit making it clear that these are the only values.

Done. I didn't add the enum/const since the element count is passed through
several functions before the Instr constructor, added an assert in the ctor
instead.

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

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:3915: // version of LoadIndexedInstr which
accepts only string targets may load
On 2014/10/07 12:18:35, zerny-google wrote:
> which accepts only string targets may load
> ->
> which accepts only string targets and can load

Done.

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:1438: case 2: __ ldrh(result,
element_address); break;
On 2014/10/07 12:18:35, zerny-google wrote:
> Nit: the fact that case 3 is not missing would be a bit more clear with the
> explicit type of code units.

Acknowledged.

https://codereview.chromium.org/539153002/diff/110001/runtime/vm/intermediate...
runtime/vm/intermediate_language_arm.cc:6993: LocationSummary* locs =
new(isolate) LocationSummary(
On 2014/10/07 12:18:35, zerny-google wrote:
> Nit: Not sure if locs or summary is the preferred naming convention. I'd stick
> with just one for your changes...

Seems to be split 1:2 (locs:summary), I went with the majority and changed all
occurrences to summary.

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm64.cc:1540: __ LoadClassId(R1, R1, PP);
On 2014/10/07 12:49:37, Florian Schneider wrote:
> I don't think you can use PP here, since the pool register is not set up like
it
> is in normal function here in intrinsic code. The FlowGraphCompiler disables
the
> constant pool for instrinsics, but better use kNoPP instead to be safe.
> 

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm64.cc:1541: __ AddImmediate(R1, R1,
-kOneByteStringCid, PP);
On 2014/10/07 12:49:37, Florian Schneider wrote:
> kNoPP here and below as well.

Done.

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_ia32.cc:2095: __ movl(ECX,
Immediate(reinterpret_cast<intptr_t>(Object::null())));
On 2014/10/07 12:49:36, Florian Schneider wrote:
> Alternatively, just store smi 0:
> 
> __ xorl(ECX, ECX);
> 
> (applies to the other architetures as well)

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.cc (right):

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:396: Array::Cast(Instance::ZoneHandle(I,
word_character_field.value())));
On 2014/10/07 12:49:37, Florian Schneider wrote:
> No need for Array::Cast here.

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:940:
InstanceCall(InstanceCallDescriptor::FromToken(Token::kADD),
On 2014/10/07 12:49:36, Florian Schneider wrote:
> Use helper IRRegExpMacroAssembler::Add here?
> 
> Also: Some helpers perform Bind/Do, others like ::InstanceCall don't.
> 
> I'd make them consistent. Maybe never perform Bind/Do inside these, so there
is
> less danger of using them where evaluation order could be wrong.
> 
> You'd need to change ::Add, ::Sub, etc. to match what ::Comparison does: Take
> Definition* as argument and Bind them inside.

As discussed in person, it is necessary to keep the order of argument pushes
explicit in these cases and thus we cannot modify
Add/Sub/InstanceCall/StaticCall to take Definition* arguments. Removed Bind()
from inside Add/Sub to stay consistent with other helpers instead.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:1760: return Bind(InstanceCall(
On 2014/10/07 12:49:36, Florian Schneider wrote:
> I'd rather not perform Bind() here, and just return Definition*

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.cc:1768: return Bind(InstanceCall(
On 2014/10/07 12:49:37, Florian Schneider wrote:
> Also here better not to call Bind() inside.

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
File runtime/vm/regexp_assembler.h (right):

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/regexp_assem...
runtime/vm/regexp_assembler.h:610: IdAllocator block_id;
On 2014/10/07 12:49:37, Florian Schneider wrote:
> Rename private members to have _ at the end
> 
> block_id_
> temp_id_
> etc...

Done.

[Florian Schneider, 2014-10-07 15:31:30]

LGTM from my side.

[Vyacheslav Egorov (Google), 2014-10-07 15:48:30]

LGTM with comments from me too

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:84: class StandardCompilationPipeline : public
CompilationPipeline {
I would call it 'Default' or 'Dart' CompilationPipeline instead of 'Standard'.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:104: virtual void CompileGraph(FlowGraphCompiler*
graph_compiler) {
Given that CompileGraph is the same in both pipelines I would just do that
outside int he Compiler..Helper. and call this function FinalizeCompilation() or
something. It will be empty by default and in RegExp it will do
FinalizeBlockOffsetTable

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:822: // Irregexp functions do not have a node
sequence.
I wonder what happens if somebody tries to break into long running regexp with a
debugger. 

If debugger stops - I wonder if it break because there are no local var
descriptors. 


We need to check what happens and potentially prohibit debugging of irregexp
functions.

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20006: case kOneByteStringCid: dest =
&raw_ptr()->one_byte_function_; break;
I suggest make formatting more uniform

case kOneByteStringCid:
  dest = ...
  break;

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20016: StorePointer(dest, value.raw());
You can make JSRegExp a friend class of Instance and then write

RawFunction** FunctionAddr(intptr_t cid) {
  return
&reinterpret_cast<RawFunction**>(FieldAddrAtOffset(function_offset(cid))); 
}

StorePointer(FunctionAddr(cid), value.raw());

and in 

RawFunction* function(intptr_t cid) const {
  return *FunctionAddr(cid);
}

so there will be a single switch statement only.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/object.h#new...
runtime/vm/object.h:7261: void set_sample_subject(intptr_t cid, const String&
value) const;
is set_sample_subject still used from somewhere?

[jgruber1, 2014-10-07 17:16:51]

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:84: class StandardCompilationPipeline : public
CompilationPipeline {
On 2014/10/07 15:48:30, Vyacheslav Egorov (Google) wrote:
> I would call it 'Default' or 'Dart' CompilationPipeline instead of 'Standard'.

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:104: virtual void CompileGraph(FlowGraphCompiler*
graph_compiler) {
On 2014/10/07 15:48:30, Vyacheslav Egorov (Google) wrote:
> Given that CompileGraph is the same in both pipelines I would just do that
> outside int he Compiler..Helper. and call this function FinalizeCompilation()
or
> something. It will be empty by default and in RegExp it will do
> FinalizeBlockOffsetTable

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:822: // Irregexp functions do not have a node
sequence.
On 2014/10/07 15:48:30, Vyacheslav Egorov (Google) wrote:
> I wonder what happens if somebody tries to break into long running regexp with
a
> debugger. 
> 
> If debugger stops - I wonder if it break because there are no local var
> descriptors. 
> 
> 
> We need to check what happens and potentially prohibit debugging of irregexp
> functions.

Done.

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

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20006: case kOneByteStringCid: dest =
&raw_ptr()->one_byte_function_; break;
On 2014/10/07 15:48:30, Vyacheslav Egorov (Google) wrote:
> I suggest make formatting more uniform
> 
> case kOneByteStringCid:
>   dest = ...
>   break;
> 

Done.

https://codereview.chromium.org/539153002/diff/120001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:20016: StorePointer(dest, value.raw());
On 2014/10/07 15:48:30, Vyacheslav Egorov (Google) wrote:
> You can make JSRegExp a friend class of Instance and then write
> 
> RawFunction** FunctionAddr(intptr_t cid) {
>   return
> &reinterpret_cast<RawFunction**>(FieldAddrAtOffset(function_offset(cid))); 
> }
> 
> StorePointer(FunctionAddr(cid), value.raw());
> 
> and in 
> 
> RawFunction* function(intptr_t cid) const {
>   return *FunctionAddr(cid);
> }
> 
> so there will be a single switch statement only.

Done.

[Ivan Posva, 2014-10-09 09:54:15]

First round of comments.

-Ivan

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

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:7: #include "vm/compiler.h"
Why is compiler.h needed here?

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:36: if (!RegExpParser::ParseRegExp(pattern, multi_line,
&compileData)) {
Is there any particular reason why the code below cannot throw the exception
while parsing. Going through the regular expression twice seems unnecessary.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:43: isolate, RegExpEngine::New(isolate, pattern,
multi_line, ignore_case));
RegExpEngine::New should not go into a JSRegExp::Handle. The first implies
allocating an object RegExpEngine.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:101: const Function& fn =
Function::Handle(regexp.function(cid));
An API based on class ids seems not the best thing to me.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp_patc...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:41: // Regular expression objects are stored in a
cache of up to _MAX_CACHE_SIZE
Please take a look at the _AccessorCache in mirrors_impl.dart for a solution
that is self-sizing.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:49: <_JSSyntaxRegExpHashKey,
_JSSyntaxRegExpValue>{ };
If you use maps which do not need insertion order iteration please use new
HashMap().

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/string_patc...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:742: // The codeUnitsAt family of functions is
inlined to an unchecked
What about external strings?

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

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:75: virtual FlowGraph* BuildFlowGraph(
Why does this BuildFlowGraph method have a different API than the
FlowGraphBuilder constructor? In particular why do you need to pass a pointer to
ic_data_array instead of a const reference?

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:988: if (function.IsIrregexpFunction()) {
CompilationPipeline* pipeline = CompilationPipeline::New(function);
return CompileFunctionHelper(pipeline, function, false, Isolate::kNoDeoptId);

Where CompilationPipeline::New allocates the appropriate Zone allocated
pipeline.

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:823: if (flow_graph().IsIrregexpFunction())
return;
if (parsed_function_.node_sequence() == NULL) {
  ASSERT(flow_graph().IsIrregexpFunction());
  return;
}

or similar

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:2882: return false;
What will be generated when we return false here?

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

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2222: // array and are looked up at runtime.
It is not clear what runtime means here. While executing the regular expression?

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:3922: LoadCodeUnitsInstr(Value* array,
Why do we need a special instruction here? Can't this be "just" a static call to
a corresponding recognized method?
Why is one of the values being passed an array? Shouldn't that be the string?

[jgruber1, 2014-10-09 13:36:17]

Fixes uploaded.

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

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:7: #include "vm/compiler.h"
On 2014/10/09 09:54:15, Ivan Posva wrote:
> Why is compiler.h needed here?

Left over from previous efforts, removed now.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:36: if (!RegExpParser::ParseRegExp(pattern, multi_line,
&compileData)) {
On 2014/10/09 09:54:15, Ivan Posva wrote:
> Is there any particular reason why the code below cannot throw the exception
> while parsing. Going through the regular expression twice seems unnecessary.

I'm not sure if we're on the same page here; regexps are parsed only once within
the factory constructor, and this needs to be done to catch invalid pattern
formats. They are then parsed again during each compilation run. We could keep
the parsed RegExpTree around in order to avoid reparsing, but it takes such a
miniscule amount of time that this seems like the better tradeoff.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:43: isolate, RegExpEngine::New(isolate, pattern,
multi_line, ignore_case));
On 2014/10/09 09:54:15, Ivan Posva wrote:
> RegExpEngine::New should not go into a JSRegExp::Handle. The first implies
> allocating an object RegExpEngine.

Renamed RegExpEngine::New to RegExpEngine::CreateJSRegExp, and removed the
Handle construction.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:101: const Function& fn =
Function::Handle(regexp.function(cid));
On 2014/10/09 09:54:15, Ivan Posva wrote:
> An API based on class ids seems not the best thing to me.

What would you like to see instead? The regexp functions are specialized based
on the class ids, so we need to retrieve them by class id.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp_patc...
File runtime/lib/regexp_patch.dart (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:41: // Regular expression objects are stored in a
cache of up to _MAX_CACHE_SIZE
On 2014/10/09 09:54:15, Ivan Posva wrote:
> Please take a look at the _AccessorCache in mirrors_impl.dart for a solution
> that is self-sizing.

Acknowledged, and added a TODO since this seems to be a good candidate for a
follow-up CL.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp_patc...
runtime/lib/regexp_patch.dart:49: <_JSSyntaxRegExpHashKey,
_JSSyntaxRegExpValue>{ };
On 2014/10/09 09:54:15, Ivan Posva wrote:
> If you use maps which do not need insertion order iteration please use new
> HashMap().

Done.

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/string_patc...
File runtime/lib/string_patch.dart (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/string_patc...
runtime/lib/string_patch.dart:742: // The codeUnitsAt family of functions is
inlined to an unchecked
On 2014/10/09 09:54:15, Ivan Posva wrote:
> What about external strings?

codeUnitsAt currently does not optimize for external strings (the same is true
for CharAt and CodeUnitAt by the way). There is a TODO In
InlineStringCodeUnitsAt about this.

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

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:75: virtual FlowGraph* BuildFlowGraph(
On 2014/10/09 09:54:15, Ivan Posva wrote:
> Why does this BuildFlowGraph method have a different API than the
> FlowGraphBuilder constructor? In particular why do you need to pass a pointer
to
> ic_data_array instead of a const reference?

Done, modified RegExpEngine::Compile to take a const reference instead and
updated the signature to match FlowGraphBuilder.

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/compiler.cc#...
runtime/vm/compiler.cc:988: if (function.IsIrregexpFunction()) {
On 2014/10/09 09:54:15, Ivan Posva wrote:
> CompilationPipeline* pipeline = CompilationPipeline::New(function);
> return CompileFunctionHelper(pipeline, function, false, Isolate::kNoDeoptId);
> 
> Where CompilationPipeline::New allocates the appropriate Zone allocated
> pipeline.

Done.

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_c...
File runtime/vm/flow_graph_compiler.cc (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_c...
runtime/vm/flow_graph_compiler.cc:823: if (flow_graph().IsIrregexpFunction())
return;
On 2014/10/09 09:54:15, Ivan Posva wrote:
> if (parsed_function_.node_sequence() == NULL) {
>   ASSERT(flow_graph().IsIrregexpFunction());
>   return;
> }
> 
> or similar

Done.

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_o...
File runtime/vm/flow_graph_optimizer.cc (right):

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/flow_graph_o...
runtime/vm/flow_graph_optimizer.cc:2882: return false;
On 2014/10/09 09:54:15, Ivan Posva wrote:
> What will be generated when we return false here?

The inlined version of String::_codeUnitsAt().

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

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2222: // array and are looked up at runtime.
On 2014/10/09 09:54:15, Ivan Posva wrote:
> It is not clear what runtime means here. While executing the regular
expression?

Yes. Clarified the comment.

https://codereview.chromium.org/539153002/diff/160001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:3922: LoadCodeUnitsInstr(Value* array,
On 2014/10/09 09:54:15, Ivan Posva wrote:
> Why do we need a special instruction here? Can't this be "just" a static call
to
> a corresponding recognized method?
> Why is one of the values being passed an array? Shouldn't that be the string?

The instruction is needed because the corresponding recognized method optimizes
to it. It's different from LoadIndexed in that it may load more than one 'array
element' (in this case, code unit from a string) at once and pack them into a
single return value.

It's one of the hottest functions in irregexp execution, so optimizing down to a
single mov (plus a SmiTag and possibly a SmiUntag) makes a big difference.

The 'array' name was left over from LoadIndexedInstr, I've changed it to 'str'.

[Ivan Posva, 2014-10-10 06:35:54]

Next round of comments.

-Ivan

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

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:36: if (!RegExpParser::ParseRegExp(pattern, multi_line,
&compileData)) {
On 2014/10/09 13:36:16, jgruber wrote:
> On 2014/10/09 09:54:15, Ivan Posva wrote:
> > Is there any particular reason why the code below cannot throw the exception
> > while parsing. Going through the regular expression twice seems unnecessary.
> 
> I'm not sure if we're on the same page here; regexps are parsed only once
within
> the factory constructor, and this needs to be done to catch invalid pattern
> formats. They are then parsed again during each compilation run. We could keep
> the parsed RegExpTree around in order to avoid reparsing, but it takes such a
> miniscule amount of time that this seems like the better tradeoff.

The comment made it sound like the compilation happens within this native call,
which apparently is not the case.

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

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2240: // and passed to IndirectGoto as an
input.
I am really interested in the security implications of this mechanism before
proceeding further.

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/object.h#new...
runtime/vm/object.h:1827: case RawFunction::kIrregexpFunction:
These two are contradictory. How can a irregexp function be static here and
dynamic above?

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:677: int16_t regexp_cid_;
Would an int8_t do? Or a uint8_t?

Generally it is a bad idea to add fields to RawFunction, as every field added
causes significant memory consumption.

It does not appear that this field is ever being properly preserved into or
initialized from a snapshot.

Also these fields should be sorted by size, so that we do not end up with
internal alignment. Not your CL's issue though.

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

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4173: JSRegExp* regexp,
const JSRegExp&

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4190: fn.set_parameter_types(
These parameter arrays should be allocated only once and shared between all
regiexp functions. They are identical among all of them.

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4218: if (multi_line) regexp.set_is_multi_line();
if (cond) {
  statements;
}

[Vyacheslav Egorov (Google), 2014-10-10 08:08:09]

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

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:2240: // and passed to IndirectGoto as an
input.
On 2014/10/10 06:35:54, Ivan Posva wrote:
> I am really interested in the security implications of this mechanism before
> proceeding further.

Security implications are as follows: if you happen to write over offsets array
and you know where start of regexp code is - then you can jump to arbitrary
address. 

So ASLR that we apply to our code pages a mitigation in itself, because it makes
harder to predict absolute address of the code object.

Additional potential mitigations are in order of simplicity:

- XORing offsets with a security cookie;
- allocating offsets array on a page with r-- permissions.

I also would like to point out that similar security implications already exist
for architectures with object pools (ARM64, X64) - we store absolute addresses
of external labels in those pools and dispatch to them directly. 

In this case ASLR is not even a mitigation - because we store an absolute
address, so if you happen to write over constant pool you can put arbitrary
target address there - and we will just jump to it.

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:677: int16_t regexp_cid_;
On 2014/10/10 06:35:54, Ivan Posva wrote:
> Would an int8_t do? Or a uint8_t?
> 
> Generally it is a bad idea to add fields to RawFunction, as every field added
> causes significant memory consumption.
> 

We were indeed concerned about the size of the Function (if you go back through
comments you will see that I was against adding any fields to it).

However here I counted the fields and saw that we have 16bits ununused - so this
field does not really increase any memory consumption.

I have a backup plan on how to *not* add this field  - which we will apply if
somebody else needs 16bits in Function. The only reason we did not apply it
right away was that it makes current CL even bigger.

> It does not appear that this field is ever being properly preserved into or
> initialized from a snapshot.
> 
> Also these fields should be sorted by size, so that we do not end up with
> internal alignment. Not your CL's issue though.

Great catch with a snapshot. Really easy to forget with the current setup, and
it's not caught statically.

[jgruber1, 2014-10-10 09:29:44]

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

https://codereview.chromium.org/539153002/diff/160001/runtime/lib/regexp.cc#n...
runtime/lib/regexp.cc:36: if (!RegExpParser::ParseRegExp(pattern, multi_line,
&compileData)) {
On 2014/10/10 06:35:53, Ivan Posva wrote:
> On 2014/10/09 13:36:16, jgruber wrote:
> > On 2014/10/09 09:54:15, Ivan Posva wrote:
> > > Is there any particular reason why the code below cannot throw the
exception
> > > while parsing. Going through the regular expression twice seems
unnecessary.
> > 
> > I'm not sure if we're on the same page here; regexps are parsed only once
> within
> > the factory constructor, and this needs to be done to catch invalid pattern
> > formats. They are then parsed again during each compilation run. We could
keep
> > the parsed RegExpTree around in order to avoid reparsing, but it takes such
a
> > miniscule amount of time that this seems like the better tradeoff.
> 
> The comment made it sound like the compilation happens within this native
call,
> which apparently is not the case.

Acknowledged.

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/object.h#new...
runtime/vm/object.h:1827: case RawFunction::kIrregexpFunction:
On 2014/10/10 06:35:54, Ivan Posva wrote:
> These two are contradictory. How can a irregexp function be static here and
> dynamic above?

Done.

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:677: int16_t regexp_cid_;
On 2014/10/10 06:35:54, Ivan Posva wrote:
> It does not appear that this field is ever being properly preserved into or
> initialized from a snapshot.

Added snapshotting of regexp_cid_.

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

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4173: JSRegExp* regexp,
On 2014/10/10 06:35:54, Ivan Posva wrote:
> const JSRegExp&

Done.

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4190: fn.set_parameter_types(
On 2014/10/10 06:35:54, Ivan Posva wrote:
> These parameter arrays should be allocated only once and shared between all
> regiexp functions. They are identical among all of them.

Where should they be cached? FYI, CreateMethodExtractor also creates new
parameter types/names arrays each time it is called. I added a TODO in the
meantime.

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4218: if (multi_line) regexp.set_is_multi_line();
On 2014/10/10 06:35:54, Ivan Posva wrote:
> if (cond) {
>   statements;
> }

Done.

[Vyacheslav Egorov (Google), 2014-10-10 09:43:25]

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

https://codereview.chromium.org/539153002/diff/180001/runtime/vm/regexp.cc#ne...
runtime/vm/regexp.cc:4190: fn.set_parameter_types(
On 2014/10/10 09:29:44, jgruber wrote:
> On 2014/10/10 06:35:54, Ivan Posva wrote:
> > These parameter arrays should be allocated only once and shared between all
> > regiexp functions. They are identical among all of them.
> 
> Where should they be cached? FYI, CreateMethodExtractor also creates new
> parameter types/names arrays each time it is called. I added a TODO in the
> meantime.

For starters they could be cached outside of this function in the caller. 

That's a small easy win, but I don't think any further engineering with caching
them globally is worth it in terms of memory consumption, unless there is a case
where 10000s of RegExps are floating around.