Last Comment Bug 451788 - TM: Multiplying, dividing, modulo two integers promotes to double
: TM: Multiplying, dividing, modulo two integers promotes to double
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: P5 enhancement (vote)
: mozilla1.9.1
Assigned To: Andreas Gal :gal
:
Mentors:
Depends on:
Blocks: 451673
  Show dependency treegraph
 
Reported: 2008-08-22 15:12 PDT by Boris Zbarsky [:bz]
Modified: 2011-10-31 11:56 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, this time hopefully working. (1.86 KB, patch)
2008-09-16 06:10 PDT, Andreas Gal :gal
brendan: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2008-08-22 15:12:00 PDT
The minimal shell testcase in bug 451673 has this line:

  limit = i * 1;

Here 'i' is an integer counter.  As far as I can tell, the tracer treats 'i' as an integer, and of course 1 is an integer, but the product is promoted to double. It might be worth it to bounds-check and not promote if we won't overflow.
Comment 1 Andreas Gal :gal 2008-08-22 15:36:29 PDT
I intentionally didn't demote this since multiplications often overflow (they do in one of the sunspider benchmarks). But yes, we should give this another try.
Comment 2 Andreas Gal :gal 2008-09-15 14:11:04 PDT
Changed title to include division as well (which implies modulo too).
Comment 3 Andreas Gal :gal 2008-09-15 15:59:57 PDT
Modulo by a non-zero constant is easy and helps 3d-raytrace:

http://hg.mozilla.org/tracemonkey/rev/87fe68f51647
Comment 4 Andreas Gal :gal 2008-09-15 17:13:19 PDT
Backed out the previous patch again until its fixed. 

http://hg.mozilla.org/tracemonkey/rev/61b9209c186f
Comment 5 Andreas Gal :gal 2008-09-16 06:10:01 PDT
Created attachment 338850 [details] [diff] [review]
Patch, this time hopefully working.
Comment 6 Brendan Eich [:brendan] 2008-09-16 06:16:18 PDT
Comment on attachment 338850 [details] [diff] [review]
Patch, this time hopefully working.

>+/*
>+ * Note: Caller is responsible for ensuring that b is not 0, or really bad things are going to
>+ *       happen.

This would look nicer if wrapped before column 80 -- most comments do.

>+          case F_dmod: 
>+            JS_ASSERT(s0->isQuad() && args[1]->isQuad());
>+            if (args[1]->isconstq() && args[1]->constvalq() && isPromote(s0)) {
>+                LIns* args2[] = { demote(out, s0), demote(out, args[1]) };
>+                return out->ins1(LIR_i2f, out->insCall(F_imod, args2));

Could you comment with a FIXME: bug NNNNNN where the bug number is the one about adding LIR_imod?

/be
Comment 7 Andreas Gal :gal 2008-09-16 06:23:38 PDT
http://hg.mozilla.org/tracemonkey/rev/b8f1727c3c5b
Comment 8 Damon Sicore (:damons) 2008-09-23 18:17:54 PDT
Not a blocker as it's an enhancement.

Note You need to log in before you can comment on or make changes to this bug.