Simplify integer arithmetic code.

Simplify integer arithmetic code.
Optimize Mint multiplication in 32-bit mode.
Committed: https://code.google.com/p/dart/source/detail?r=3417

[Ivan Posva, 2012-01-16 18:08:41]

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode258
runtime/lib/integers.cc:258: break;  // Perform a Bigint multiplication below.
Move the break all the way out, so that there is no path leading into the
division below.

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode274
runtime/lib/integers.cc:274: }
Fall-through potential!

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode284
runtime/lib/integers.cc:284: const int64_t limit = (static_cast<int64_t>(1)) <<
62;
Why not use Utils::IsInt(63, left_value) here?

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode295
runtime/lib/integers.cc:295: const int64_t limit = (static_cast<int64_t>(1)) <<
31;
Above you use Utils::IsInt(32, left_value), but not here?

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode300
runtime/lib/integers.cc:300: break;  // Perform a Bigint multiplication below.
ditto. Please ensure that no path leads to the fall-through, by moving this out
of all conditionals.

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode314
runtime/lib/integers.cc:314: return Integer::New(remainder);
Should not be inside a conditional as well.

[regis, 2012-01-17 17:50:04]

Thanks.

Let me know if there is a good reason not to change the assert in Utils::IsInt.

-- Regis

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode258
runtime/lib/integers.cc:258: break;  // Perform a Bigint multiplication below.
On 2012/01/16 18:08:41, Ivan Posva wrote:
> Move the break all the way out, so that there is no path leading into the
> division below.

Done here and below.

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode274
runtime/lib/integers.cc:274: }
On 2012/01/16 18:08:41, Ivan Posva wrote:
> Fall-through potential!

I have removed the 'else' and terminated the case with the 'return' statement.
Done here and below.

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode284
runtime/lib/integers.cc:284: const int64_t limit = (static_cast<int64_t>(1)) <<
62;
On 2012/01/16 18:08:41, Ivan Posva wrote:
> Why not use Utils::IsInt(63, left_value) here?

Because of the ASSERT(N < kBitsPerWord) in Utils::IsInt. Note that we execute
this code in 32-bit mode.

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode295
runtime/lib/integers.cc:295: const int64_t limit = (static_cast<int64_t>(1)) <<
31;
On 2012/01/16 18:08:41, Ivan Posva wrote:
> Above you use Utils::IsInt(32, left_value), but not here?

This code is executed in 32-bit mode, therefore ASSERT(N < kBitsPerWord) fails.
The code above is only executed in 64-bit mode, so the assert is fine.
This is ugly. Agreed. Why do we need this assert in Utils::IsInt? If any, it
should be N < 2*kBitsPerWord.

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode300
runtime/lib/integers.cc:300: break;  // Perform a Bigint multiplication below.
On 2012/01/16 18:08:41, Ivan Posva wrote:
> ditto. Please ensure that no path leads to the fall-through, by moving this
out
> of all conditionals.

Done.

http://codereview.chromium.org/9112050/diff/1/runtime/lib/integers.cc#newcode314
runtime/lib/integers.cc:314: return Integer::New(remainder);
On 2012/01/16 18:08:41, Ivan Posva wrote:
> Should not be inside a conditional as well.

Done.

[Ivan Posva, 2012-01-18 20:05:21]

http://codereview.chromium.org/9112050/diff/12001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9112050/diff/12001/runtime/lib/integers.cc#new...
runtime/lib/integers.cc:257: } else {
This empty, comment-only else looks weird in my eyes. The comment could just
move to straight above the break.

http://codereview.chromium.org/9112050/diff/12001/runtime/lib/integers.cc#new...
runtime/lib/integers.cc:297: // Perform a Bigint multiplication below.
ditto

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h
File runtime/platform/utils.h (right):

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:91: ASSERT((0 < N) && (N < (kBitsPerByte *
sizeof(T))));
sizeof(value) is closer to the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#sizeof

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:92: word limit = static_cast<word>(1) << (N - 1);
You need to cast the 1 to T instead of word here. And the type of limit needs to
change as well. Can you please add the appropriate tests for 64-bit values which
would have failed here on a 32-bit platform. Thanks!

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:96: template<typename T>
ditto

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:105: template<typename T>
ditto

[regis, 2012-01-18 21:50:12]

Thanks

http://codereview.chromium.org/9112050/diff/12001/runtime/lib/integers.cc
File runtime/lib/integers.cc (right):

http://codereview.chromium.org/9112050/diff/12001/runtime/lib/integers.cc#new...
runtime/lib/integers.cc:257: } else {
On 2012/01/18 20:05:22, Ivan Posva wrote:
> This empty, comment-only else looks weird in my eyes. The comment could just
> move to straight above the break.

Done.

http://codereview.chromium.org/9112050/diff/12001/runtime/lib/integers.cc#new...
runtime/lib/integers.cc:297: // Perform a Bigint multiplication below.
On 2012/01/18 20:05:22, Ivan Posva wrote:
> ditto

Done.

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h
File runtime/platform/utils.h (right):

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:91: ASSERT((0 < N) && (N < (kBitsPerByte *
sizeof(T))));
On 2012/01/18 20:05:22, Ivan Posva wrote:
> sizeof(value) is closer to the style guide:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#sizeof

Done.

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:92: word limit = static_cast<word>(1) << (N - 1);
On 2012/01/18 20:05:22, Ivan Posva wrote:
> You need to cast the 1 to T instead of word here. And the type of limit needs
to
> change as well. Can you please add the appropriate tests for 64-bit values
which
> would have failed here on a 32-bit platform. Thanks!

The cast is fixed in patch 5.
I have added a test.

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:96: template<typename T>
On 2012/01/18 20:05:22, Ivan Posva wrote:
> ditto

Done.

http://codereview.chromium.org/9112050/diff/12001/runtime/platform/utils.h#ne...
runtime/platform/utils.h:105: template<typename T>
On 2012/01/18 20:05:22, Ivan Posva wrote:
> ditto

Done.

[Ivan Posva, 2012-01-18 21:59:40]

LGTM -ip