Note: There are a few cases of duplicates in user autocompletion which are being worked on.

TM: Multiplying, dividing, modulo two integers promotes to double

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
JavaScript Engine
P5
enhancement
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: bz, Assigned: gal)

Tracking

unspecified
mozilla1.9.1
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
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.

Updated

9 years ago
Severity: normal → enhancement
Priority: -- → P5
Target Milestone: --- → mozilla1.9.1
(Assignee)

Updated

9 years ago
Summary: TM: Multiplying two integers promotes to double → TM: Multiplying and dividing two integers promotes to double
(Assignee)

Comment 2

9 years ago
Changed title to include division as well (which implies modulo too).
(Assignee)

Updated

9 years ago
Summary: TM: Multiplying and dividing two integers promotes to double → TM: Multiplying, dividing, modulo two integers promotes to double
(Assignee)

Comment 3

9 years ago
Modulo by a non-zero constant is easy and helps 3d-raytrace:

http://hg.mozilla.org/tracemonkey/rev/87fe68f51647
(Assignee)

Comment 4

9 years ago
Backed out the previous patch again until its fixed. 

http://hg.mozilla.org/tracemonkey/rev/61b9209c186f
(Assignee)

Comment 5

9 years ago
Created attachment 338850 [details] [diff] [review]
Patch, this time hopefully working.
Assignee: general → gal
Attachment #338850 - Flags: review?(brendan)
Attachment #338850 - Flags: review?(brendan) → review+
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
(Assignee)

Comment 7

9 years ago
http://hg.mozilla.org/tracemonkey/rev/b8f1727c3c5b
Not a blocker as it's an enhancement.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.