Closed Bug 584838 Opened 11 years ago Closed 11 years ago

JM: jsop_neg on an int32 constant should create an int, not double

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.5 (KHTML, like Gecko) Chrome/6.0.486.0 Safari/534.5
Build Identifier: 

jsop_neg converts constants to double. I think we should convert to int, if possible. This will in faster code for other operators.

Patch coming..

Reproducible: Always
Attached patch Patch (obsolete) — Splinter Review
The patch is a clear win on SS and V8, not sure how much - numbers are a bit 
unreliable on this machine.

Results for a micro-benchmark:
-------
for (var i = 0; i < 1000000; i++) {
  var z = 123;
  res += -z + x;
}
-------

JM old: 36 ms.
JM new: 8 ms.
TM: 6 ms.
Attachment #463268 - Flags: review?(dvander)
Comment on attachment 463268 [details] [diff] [review]
Patch

Awesome, Jan! Thanks for your work.

>+                if(top->isType(JSVAL_TYPE_INT32) && 
>+                        top->getValue().toInt32() != 0 &&
>+                        top->getValue().toInt32() != (1 << 31)) {

House style looks like:

>+                if (top->isType(JSVAL_TYPE_INT32) && 
>+                    top->getValue().toInt32() != 0 &&
>+                    top->getValue().toInt32() != (1 << 31)) {
Attachment #463268 - Flags: review?(dvander) → review+
Assignee: general → jandemooij
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: checkin-needed
Attached patch Patch v2Splinter Review
Thanks. I added a newline below the if-condition, it looks a bit dense otherwise. If this is okay, can you commit it?
Attachment #463268 - Attachment is obsolete: true
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/1c815abcb498

You're right, it does look dense. In this case we move the open brace { to the new line.

Thanks again!
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
The above patch can be improved somewhat: consider the case of negating the constant (-0). Since that is stored as a double, the constant-folding logic will calculate 0, but store the result as a double, when it could really be an integer.

This patch adds a check for whether the result of a double constant-folding operating can be stored as an int32, and if it can, does so.
Attachment #463365 - Flags: review?(dvander)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Actually, it occurs to me that we can just do all negation in the double-space, and then just check whether the result is representable as an integer.

This patch undoes the previous patch and adds the conversion.
Attachment #463365 - Attachment is obsolete: true
Attachment #463367 - Flags: review?(dvander)
Attachment #463365 - Flags: review?(dvander)
Comment on attachment 463367 [details] [diff] [review]
Try conversion to int32, v2

Can't this be NumberValue(d) ?
Attachment #463367 - Flags: review?(dvander) → review+
http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/afb2ae0fdb30

Indeed it can! Thanks, Jan.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.