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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

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.

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

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)) {

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch v2

Thanks. I added a newline below the if-condition, it looks a bit dense otherwise. If this is okay, can you commit it?

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

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!

Comment 5

[Sean Stangl [:sstangl]]

Attachment: Try converting jsop_neg() constant doubles to int32.

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.

Comment 6

[Sean Stangl [:sstangl]]

Attachment: Try conversion to int32, v2

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.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 463367 [details] [diff] [review]
Try conversion to int32, v2

Can't this be NumberValue(d) ?

Comment 8

[Sean Stangl [:sstangl]]

http://hg.mozilla.org/users/danderson_mozilla.com/moo/rev/afb2ae0fdb30

Indeed it can! Thanks, Jan.