Infer the range of Math.abs

Infer the range of Math.abs

R=jkummerow@chromium.org, svenpanne@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=14996

[Toon Verwaest, 2013-06-06 18:00:47]

PTAL

[Sven Panne, 2013-06-07 06:40:54]

DBC

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:274: void TruncateToSmi() {
I think that "ClampToSmi" is a better name. "Clamping" is the common name for
this kind of operation, "truncation" is more associated with floating point
stuff.

[Sven Panne, 2013-06-07 08:11:47]

LGTM with a nit (drive-by, after heated discussions ;-)

https://codereview.chromium.org/16268009/diff/5001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/16268009/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1326: Range* result = new(zone) Range(lower,
upper);
Shorter and more readable:

new(zone) Range(spans_zero ? 0 : Min(Abs(lower), Abs(upper)),
                Max(Abs(lower), Abs(upper)))

[Toon Verwaest, 2013-06-07 08:18:02]

Addressed comment. Landing.

https://codereview.chromium.org/16268009/diff/5001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/16268009/diff/5001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1326: Range* result = new(zone) Range(lower,
upper);
On 2013/06/07 08:11:48, Sven Panne wrote:
> Shorter and more readable:
> 
> new(zone) Range(spans_zero ? 0 : Min(Abs(lower), Abs(upper)),
>                 Max(Abs(lower), Abs(upper)))

Done.

[Jakob Kummerow, 2013-06-07 08:21:05]

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:1317: bool spans_zero = lower < 0 && 0 < upper;
There's Range::CanBeZero(). It checks for '<=' instead of '<' for both
relations, but that shouldn't make a difference here.

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:1327: if (r.IsSmi()) result->TruncateToSmi();
Is this needed? Doesn't r.IsSmi() imply value()->representation().IsSmi(), which
in turn implies that value()->range() is within Smi range anyway?

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.h:278: void TruncateToInt32() {
Considering that lower_ and upper_ are int32_t, this function is always a no-op.
I don't think we need no-op functions :-)

https://chromiumcodereview.appspot.com/16268009/diff/11001/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/16268009/diff/11001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1317: bool spans_zero = lower < 0 && 0 < upper;
There's Range::CanBeZero(). It checks for '<=' instead of '<' for both
relations, but that shouldn't make a difference here.

https://chromiumcodereview.appspot.com/16268009/diff/11001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1321: if (r.IsSmi()) result->ClampToSmi();
Is this needed? Doesn't r.IsSmi() imply value()->representation().IsSmi(), which
in turn implies that value()->range() is within Smi range anyway?

https://chromiumcodereview.appspot.com/16268009/diff/11001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/16268009/diff/11001/src/hydrogen-instr...
src/hydrogen-instructions.h:278: void ClampToInt32() {
Considering that lower_ and upper_ are int32_t, this function is always a no-op.
I don't think we need no-op functions :-)

[Toon Verwaest, 2013-06-07 09:00:47]

Addressed comments, PTAL again

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:1317: bool spans_zero = lower < 0 && 0 < upper;
On 2013/06/07 08:21:05, Jakob wrote:
> There's Range::CanBeZero(). It checks for '<=' instead of '<' for both
> relations, but that shouldn't make a difference here.

Done.

https://chromiumcodereview.appspot.com/16268009/diff/1/src/hydrogen-instructi...
src/hydrogen-instructions.cc:1327: if (r.IsSmi()) result->TruncateToSmi();
Added a comment to explain it.

On 2013/06/07 08:21:05, Jakob wrote:
> Is this needed? Doesn't r.IsSmi() imply value()->representation().IsSmi(),
which
> in turn implies that value()->range() is within Smi range anyway?

[Jakob Kummerow, 2013-06-07 09:02:33]

LGTM with one more comment.

https://chromiumcodereview.appspot.com/16268009/diff/16001/src/hydrogen-instr...
File src/hydrogen-instructions.cc (right):

https://chromiumcodereview.appspot.com/16268009/diff/16001/src/hydrogen-instr...
src/hydrogen-instructions.cc:1320: int abs_lower = lower == kMinInt ? kMaxInt :
abs(kMinInt);
I think you need to do this same trick for abs_upper, or is it guaranteed that
upper != kMinInt?

[Toon Verwaest, 2013-06-07 09:06:22]

Hopefully now it works :)

[Jakob Kummerow, 2013-06-07 09:07:21]

lgtm

[Toon Verwaest, 2013-06-07 09:07:59]

Committed patchset #5 manually as r14996 (presubmit successful).