Closed Bug 1085680 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(3 files)

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'.
Oops, disregard comment 0, Math.min/max should *take* signed (and continue to return signed) because they only perform ToNumber() on their arguments.
Fortunately, this doesn't break any existing released asm.js, afaics.
Attachment #8509542 - Flags: review?(benj)
Summary: OdinMonkey: tweak return type of Math.min/max → OdinMonkey: change argument type of Math.min/max from 'int' to 'signed'
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!).
Attachment #8509542 - Flags: review?(benj) → review+
Good point.  I was wanting to test out-of-int32-range; I'll add 2^31+1.
Attached patch rm-math-ret-typeSplinter Review
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.
Attachment #8509571 - Flags: review?(benj)
Attached patch tweak-clz32-sigSplinter Review
This patch makes the return type of clz32 fixnum instead.
Attachment #8509572 - Flags: review?(benj)
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 ;)
Attachment #8509572 - Flags: review?(benj) → review+
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
Attachment #8509571 - Flags: review?(benj) → review+
https://hg.mozilla.org/mozilla-central/rev/6b7fc6fbf7d7
https://hg.mozilla.org/mozilla-central/rev/a26b920f971a
https://hg.mozilla.org/mozilla-central/rev/f9ecf6812b92
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Time frame for re-bustage in nightly seems to be occurring around the timeframe of this patch.
seems https://bugzilla.mozilla.org/show_bug.cgi?id=1042823 has re-landed. That patch caused the regression last time.
You need to log in before you can comment on or make changes to this bug.