1085680 - OdinMonkey: change argument type of Math.min/max from 'int' to 'signed'

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Luke Wagner [:luke]]

After bug 1052325, callsite coercions are not required for stdlib functions, so Math.min/max should return 'int' for 'int' arguments, not 'signed'.  Also, since the value of clz32 is in the range [0, 32], it could return 'fixnum'.

Comment 1

[Luke Wagner [:luke]]

Oops, disregard comment 0, Math.min/max should *take* signed (and continue to return signed) because they only perform ToNumber() on their arguments.

Comment 2

[Luke Wagner [:luke]]

Attachment: tweak-min-max-sig

Fortunately, this doesn't break any existing released asm.js, afaics.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8509542 [details] [diff] [review]
tweak-min-max-sig

Review of attachment 8509542 [details] [diff] [review]:
-----------------------------------------------------------------

Roger that.

::: js/src/jit-test/tests/asm.js/testMathLib.js
@@ +67,5 @@
>  
>  var doubleNumbers = [NaN, Infinity, -Infinity, -10000, -3.4, -0, 0, 3.4, 10000];
>  var floatNumbers = [];
>  for (var x of doubleNumbers) floatNumbers.push(Math.fround(x));
> +var intNumbers = [-Math.pow(2,31), -10000, -3, -1, 0, 3, 10000, Math.pow(2,31)];

cause of the |0 in coercedMin, you're testing twice -Math.pow(2,31) === INT32_MIN, as Math.pow(2,31) === INT32_MAX + 1 so Math.pow(2,31)|0 === INT32_MIN. I suppose you meant Math.pow(2,31) - 1. Then I guess you don't need coercedMin / coercedMax anymore?

Unless that was your intent to test explicitly out of int32 range values, in which case i'd recommend testing another BigUnsigned, e.g. Math.pow(2, 31) + 1, just for not testing twice the same value (nitpicking!).

Comment 4

[Luke Wagner [:luke]]

Good point.  I was wanting to test out-of-int32-range; I'll add 2^31+1.

Comment 5

[Luke Wagner [:luke]]

Attachment: rm-math-ret-type

While writing the next patch I noticed that MathRetType isn't very useful anymore.  We used to switch on it but then the CoerceResult refactoring generalized that so we switch on RetType instead.

Comment 6

[Luke Wagner [:luke]]

Attachment: tweak-clz32-sig

This patch makes the return type of clz32 fixnum instead.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8509572 [details] [diff] [review]
tweak-clz32-sig

Review of attachment 8509572 [details] [diff] [review]:
-----------------------------------------------------------------

Cool

::: js/src/jit-test/tests/asm.js/testMathLib.js
@@ +69,5 @@
>  
>  var doubleNumbers = [NaN, Infinity, -Infinity, -10000, -3.4, -0, 0, 3.4, 10000];
>  var floatNumbers = [];
>  for (var x of doubleNumbers) floatNumbers.push(Math.fround(x));
> +var intNumbers = [-Math.pow(2,31), -10000, -3, -1, 0, 3, 10000, Math.pow(2,31), Math.pow(2,31)+1];

shouldn't this be in the other patch? not a big deal, of course ;)

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8509571 [details] [diff] [review]
rm-math-ret-type

Review of attachment 8509571 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4900,5 @@
>  }
>  
>  typedef Vector<MDefinition*, 4, SystemAllocPolicy> DefinitionVector;
>  
>  namespace {  

while you're there, trailing namespace here please

Comment 9

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7fc6fbf7d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/a26b920f971a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9ecf6812b92

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6b7fc6fbf7d7
https://hg.mozilla.org/mozilla-central/rev/a26b920f971a
https://hg.mozilla.org/mozilla-central/rev/f9ecf6812b92

Comment 11

[Grant Galitz]

any way this would have touched ionmonkey code? I'm seeing a re-breakage and happening of https://bugzilla.mozilla.org/show_bug.cgi?id=1054169 again

Comment 12

[Grant Galitz]

Time frame for re-bustage in nightly seems to be occurring around the timeframe of this patch.

Comment 13

[Grant Galitz]

seems https://bugzilla.mozilla.org/show_bug.cgi?id=1042823 has re-landed. That patch caused the regression last time.