Inline integer modulo operation.

Inline integer modulo operation.

R=zra@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30075

[srdjan, 2013-11-06 23:36:54]

Noticed speedups in Smi.toString.

[sra1, 2013-11-07 00:50:22]

DBC

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:2468: __ j(ZERO, deopt);
This should be unnecessary in cases where right == 10.
Does range information tell you this?

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:2488: __ j(GREATER_EQUAL, &done,
Assembler::kNearJump);
You might be able to test whether this is necessary.
The loop in Smi.toString has the guard "while (val > 0)" and right is the
constant 10 in "val % 10".  Does range information tell you this?

I believe the untagging and re-tagging is unnecessary, at least in the toString
case where both inputs are non-negative.

These comments mostly apply to "val ~/ 10" in the loop too - i.e. it might be
possible to remove three deopt checks and 5 tagging operations from the loop.

[srdjan, 2013-11-07 05:26:51]

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:2468: __ j(ZERO, deopt);
On 2013/11/07 00:50:23, sra1 wrote:
> This should be unnecessary in cases where right == 10.
> Does range information tell you this?

Yes. I had the code in and have removed it in order to make the Cl simpler. The
cost of a division is so high that I could not measure any benefit by skipping
the tests here and below. I may try more measurements later. Having less
variations makes me more certain that the code is being tested.

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:2488: __ j(GREATER_EQUAL, &done,
Assembler::kNearJump);
On 2013/11/07 00:50:23, sra1 wrote:
> You might be able to test whether this is necessary.
> The loop in Smi.toString has the guard "while (val > 0)" and right is the
> constant 10 in "val % 10".  Does range information tell you this?
> 
> I believe the untagging and re-tagging is unnecessary, at least in the
toString
> case where both inputs are non-negative.
> 
> These comments mostly apply to "val ~/ 10" in the loop too - i.e. it might be
> possible to remove three deopt checks and 5 tagging operations from the loop.

Ditto. I have not used the val range, but the range of the right side. Have not
measured any benefits, but may add the appropriate guards later.

[zra, 2013-11-07 16:40:06]

lgtm

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:2489: __ Asr(IP, right, kSmiTagSize); 
// SmiUntag right into IP.
IntegerDivide doesn't clobber temp and IP here, so you could reuse them, but
maybe it's safer to reload in case IntegerDivide changes. Another option is to
add a comment to IntegerDivide stating the integer register inputs must be
preserved.

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:2532: __ CompareImmediate(result,
Immediate(0x4000000000000000), PP);
I couldn't find an existing test for this case. Is there one? Ditto for this
corner case on other architectures.

[srdjan, 2013-11-07 21:26:10]

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_arm.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_arm.cc:2489: __ Asr(IP, right, kSmiTagSize); 
// SmiUntag right into IP.
On 2013/11/07 16:40:06, zra wrote:
> IntegerDivide doesn't clobber temp and IP here, so you could reuse them, but
> maybe it's safer to reload in case IntegerDivide changes. Another option is to
> add a comment to IntegerDivide stating the integer register inputs must be
> preserved.

I think it is safe to assume that IP is not guaranteed to be preserved across
compound assembly instructions. The cost is minimal, compared to cost of
division, I think.

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_x64.cc:2532: __ CompareImmediate(result,
Immediate(0x4000000000000000), PP);
On 2013/11/07 16:40:06, zra wrote:
> I couldn't find an existing test for this case. Is there one? Ditto for this
> corner case on other architectures.

Copy and paste error, removing. The other architectures have that check removed
(needed for TRUNCDIV).

[srdjan, 2013-11-07 21:50:33]

Committed patchset #6 manually as r30075 (presubmit successful).

[sra1, 2013-11-08 01:40:15]

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:2468: __ j(ZERO, deopt);
On 2013/11/07 05:26:52, srdjan wrote:
> On 2013/11/07 00:50:23, sra1 wrote:
> > This should be unnecessary in cases where right == 10.
> > Does range information tell you this?
> 
> Yes. I had the code in and have removed it in order to make the Cl simpler.
The
> cost of a division is so high that I could not measure any benefit by skipping
> the tests here and below. I may try more measurements later. Having less
> variations makes me more certain that the code is being tested.

I see.  The latency of idiv covers everything else in the loop.
Probably the only way left to win is to merge "val ~/ 10" and "val % 10" into a
single idiv.

[srdjan, 2013-11-08 16:02:53]

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
File runtime/vm/intermediate_language_ia32.cc (right):

https://codereview.chromium.org/61123003/diff/230001/runtime/vm/intermediate_...
runtime/vm/intermediate_language_ia32.cc:2468: __ j(ZERO, deopt);
On 2013/11/08 01:40:16, sra1 wrote:
> On 2013/11/07 05:26:52, srdjan wrote:
> > On 2013/11/07 00:50:23, sra1 wrote:
> > > This should be unnecessary in cases where right == 10.
> > > Does range information tell you this?
> > 
> > Yes. I had the code in and have removed it in order to make the Cl simpler.
> The
> > cost of a division is so high that I could not measure any benefit by
skipping
> > the tests here and below. I may try more measurements later. Having less
> > variations makes me more certain that the code is being tested.
> 
> I see.  The latency of idiv covers everything else in the loop.
> Probably the only way left to win is to merge "val ~/ 10" and "val % 10" into
a
> single idiv.

Yes, preliminary measurement show a big benefit of merging div/mod. That is what
I am (finally) working on next.