Intrinsify _StringBase._substringMatches to speedup indexOf/startsWith/endsWith under precompilatio…

Intrinsify _StringBase._substringMatches to speedup indexOf/startsWith/endsWith under precompilation.

Fix some Smi checks in other intrinsics on MIPS.

Raspberry Pi 2
Before
JIT StarryStrings(RunTime): 1851875.5 us.
noopt StarryStrings(RunTime): 23655235.0 us.

After
JIT StarryStrings(RunTime): 1829766.5 us.
noopt StarryStrings(RunTime): 8660442.0 us. (2.7x)

R=fschneider@google.com

Committed: https://github.com/dart-lang/sdk/commit/df72a27f23aa793ce4c27dca5dbec0dca2a2179d

[rmacnak, 2015-11-17 22:01:59]

rmacnak@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2015-11-18 10:26:03]

LGTM.

Please make sure that tests cover all variants (1- and 2-byte strings).

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

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1646: __ cmp(R3, Operand(R9/*otherlength*/));
s/otherlength/other.length/

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

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:1607: __ cmpq(R9, Immediate(Smi::Value(0)));
testq(R9, R9)

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:1611: __ cmpq(RBX, Immediate(Smi::Value(0)));
alternative - I saw gcc doing this:

__ testq(RBX, RBX);
__ j(SIGN, return false);

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:1625: Label loop;
Applies to all archs:

Maybe you can move the condition to the end of the loop and branch to to the
condition from here to save one branch per iteration?

Label cond;
__ jmp(&cond);
__ Bind(&loop);

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_x64.cc:1655: __ addq(R11, Immediate(1));
Move condition to here:

__ Bind(&cond);
__ cmpq(R11, R9);
__ j(LESS, &loop);
__ jmp(&return_true);

[srdjan, 2015-11-18 17:52:53]

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

[srdjan, 2015-11-18 17:52:54]

DBC

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

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1671: // Clobbers ARGS_DESCRIPTOR_REG. Safe
because this method has no optional
s/ARGS_DESCRIPTOR_REG/ARGS_DESC_REG/

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1673: void
Intrinsifier::StringBaseSubstringMatches(Assembler* assembler) {
You could push R4 (arg_desc) and restore it when returning, to be on the safe
side about argument descriptor.

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1680: __ b(&fall_through, NE);  // Index is not a
Smi.
'start' is not a smi

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1682: __ CompareClassId(R2, kOneByteStringCid,
R3);
Add somewhere (top of function?): 

Supports one- or two- byte strings as 'this', one-byte strings as others.

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:1694: __ Bind(&try_two_byte);
s/try_two_byte/try_two_byte_this/

[srdjan, 2015-11-18 17:57:12]

One more comment:

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

https://codereview.chromium.org/1450663002/diff/80001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_ia32.cc:1746: // For precompilation / not enough
registers.
I would remove 'not enough registers', and just leave: needed for
precompilation, not implemented in ia32.

[rmacnak, 2015-11-18 22:33:32]

Uri parsing tests (and StarryStrings) include the 2 byte receiver case.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
File runtime/vm/intrinsifier_arm.cc (right):

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_arm.cc:1673: void
Intrinsifier::StringBaseSubstringMatches(Assembler* assembler) {
On 2015/11/18 17:52:54, srdjan wrote:
> You could push R4 (arg_desc) and restore it when returning, to be on the safe
> side about argument descriptor.

Done.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_arm.cc:1680: __ b(&fall_through, NE);  // Index is not a
Smi.
On 2015/11/18 17:52:54, srdjan wrote:
> 'start' is not a smi

Done.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_arm.cc:1682: __ CompareClassId(R2, kOneByteStringCid,
R3);
On 2015/11/18 17:52:54, srdjan wrote:
> Add somewhere (top of function?): 
> 
> Supports one- or two- byte strings as 'this', one-byte strings as others.

Done.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
File runtime/vm/intrinsifier_ia32.cc (right):

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_ia32.cc:1746: // For precompilation / not enough
registers.
On 2015/11/18 17:57:12, srdjan wrote:
> I would remove 'not enough registers', and just leave: needed for
> precompilation, not implemented in ia32.

Done.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
File runtime/vm/intrinsifier_x64.cc (right):

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_x64.cc:1607: __ cmpq(R9, Immediate(Smi::Value(0)));
On 2015/11/18 10:26:03, Florian Schneider wrote:
> testq(R9, R9)

Done.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_x64.cc:1611: __ cmpq(RBX, Immediate(Smi::Value(0)));
On 2015/11/18 10:26:03, Florian Schneider wrote:
> alternative - I saw gcc doing this:
> 
> __ testq(RBX, RBX);
> __ j(SIGN, return false);

Neat.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_x64.cc:1625: Label loop;
On 2015/11/18 10:26:03, Florian Schneider wrote:
> Applies to all archs:
> 
> Maybe you can move the condition to the end of the loop and branch to to the
> condition from here to save one branch per iteration?
> 
> Label cond;
> __ jmp(&cond);
> __ Bind(&loop);

We can even drop the first check as it is handled by the earlier bailout of
other.length == 0.

https://chromiumcodereview.appspot.com/1450663002/diff/80001/runtime/vm/intri...
runtime/vm/intrinsifier_x64.cc:1655: __ addq(R11, Immediate(1));
On 2015/11/18 10:26:03, Florian Schneider wrote:
> Move condition to here:
> 
> __ Bind(&cond);
> __ cmpq(R11, R9);
> __ j(LESS, &loop);
> __ jmp(&return_true);

Done.

[rmacnak, 2015-11-18 22:38:43]

Committed patchset #6 (id:100001) manually as
df72a27f23aa793ce4c27dca5dbec0dca2a2179d (presubmit successful).