Intrinsify Bigint 'multiply and accumulate" function on ia32.

Intrinsify Bigint 'multiply and accumulate" function on ia32.

R=srdjan@google.com

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

[regis, 2014-09-18 23:41:23]

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

[srdjan, 2014-09-19 00:26:08]

LGTM

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart
File runtime/lib/bigint.dart (right):

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart#newc...
runtime/lib/bigint.dart:67: static Uint32List _MulAddArgs = new Uint32List(2);
add final

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart#newc...
runtime/lib/bigint.dart:861: for (i = 0; i < a_used; ++i) {
Maybe add a comment: Using Uint32List for argument passing in order to prevent
Mint allocation? (also on places below).

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart#newc...
runtime/lib/bigint.dart:905: 
In Dart file we have most often 1 empty line between functions.

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

https://codereview.chromium.org/582963003/diff/1/runtime/vm/intrinsifier_arm....
runtime/vm/intrinsifier_arm.cc:897: void Intrinsifier::Bigint_mulAdd(Assembler*
assembler) {
Just add a TODO(regis): ... (also in other places).

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

https://codereview.chromium.org/582963003/diff/1/runtime/vm/intrinsifier_ia32...
runtime/vm/intrinsifier_ia32.cc:915: void Intrinsifier::Bigint_mulAdd(Assembler*
assembler) {
Please test with --no-intrinsify, otherwise the dart method will never be tested
since the intrinsic does not have a fall through.

https://codereview.chromium.org/582963003/diff/1/runtime/vm/intrinsifier_ia32...
runtime/vm/intrinsifier_ia32.cc:1004: __ movl(FieldAddress(EAX,
TypedData::data_offset() + kWordSize), ECX);
Load EAX with null(), just in case (as suggested by you offline).

[regis, 2014-09-19 02:01:35]

Thanks!

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart
File runtime/lib/bigint.dart (right):

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart#newc...
runtime/lib/bigint.dart:67: static Uint32List _MulAddArgs = new Uint32List(2);
On 2014/09/19 00:26:08, srdjan wrote:
> add final

Done.

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart#newc...
runtime/lib/bigint.dart:861: for (i = 0; i < a_used; ++i) {
On 2014/09/19 00:26:08, srdjan wrote:
> Maybe add a comment: Using Uint32List for argument passing in order to prevent
> Mint allocation? (also on places below).

I've added a comment at the declaration of _mulAdd:
  // Note: Passing single digits as elements of args prevents Mint allocation.

There is already one at the declaration of _MulAddArgs.

https://codereview.chromium.org/582963003/diff/1/runtime/lib/bigint.dart#newc...
runtime/lib/bigint.dart:905: 
On 2014/09/19 00:26:08, srdjan wrote:
> In Dart file we have most often 1 empty line between functions.

Done.

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

https://codereview.chromium.org/582963003/diff/1/runtime/vm/intrinsifier_arm....
runtime/vm/intrinsifier_arm.cc:897: void Intrinsifier::Bigint_mulAdd(Assembler*
assembler) {
On 2014/09/19 00:26:08, srdjan wrote:
> Just add a TODO(regis): ... (also in other places).

Done.

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

https://codereview.chromium.org/582963003/diff/1/runtime/vm/intrinsifier_ia32...
runtime/vm/intrinsifier_ia32.cc:915: void Intrinsifier::Bigint_mulAdd(Assembler*
assembler) {
On 2014/09/19 00:26:08, srdjan wrote:
> Please test with --no-intrinsify, otherwise the dart method will never be
tested
> since the intrinsic does not have a fall through. 

Added
// TODO(regis): Once this intrinsic is implemented on all architectures, the
// corresponding Dart method will be untested. Add a test with --no-intrinsify.

https://codereview.chromium.org/582963003/diff/1/runtime/vm/intrinsifier_ia32...
runtime/vm/intrinsifier_ia32.cc:1004: __ movl(FieldAddress(EAX,
TypedData::data_offset() + kWordSize), ECX);
On 2014/09/19 00:26:08, srdjan wrote:
> Load EAX with null(), just in case (as suggested by you offline).

I have instead added a TODO to confirm that this is not necessary. I really do
not think it is, in the same way as it is not necessary to check argument
values. Every cycle counts in this function :-)

[regis, 2014-09-19 02:05:09]

Committed patchset #3 (id:40001) manually as r40485 (presubmit successful).