Closed Bug 605858 Opened 9 years ago Closed 9 years ago

TM: Trace inc() on all primitives

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

This came up for undefined on jsperf.com in bug 605486, but just doing all primitives is easy enough.
Comment on attachment 484715 [details] [diff] [review]
Trace inc() for all primitive values, not just numbers.

I used insImmD in the null case because that's what the number case used to do.... but should they both use insImmI?  Or is that handled elsewhere?
Attachment #484715 - Flags: review?(dvander)
Blocks: 605486
Comment on attachment 484715 [details] [diff] [review]
Trace inc() for all primitive values, not just numbers.

insImmD is right, integers shouldn't leak into the LIR (doubles are promoted to integers in a forward pipeline).
Attachment #484715 - Flags: review?(dvander) → review+
Comment on attachment 484715 [details] [diff] [review]
Trace inc() for all primitive values, not just numbers.

Requesting approval.  This is pretty safe, and is needed (barring site changes) for jsperf.com to trace.
Attachment #484715 - Flags: approval2.0?
Whiteboard: [need review] → [need approval]
Comment on attachment 484715 [details] [diff] [review]
Trace inc() for all primitive values, not just numbers.

>+    // XXXbz can we manage to do this for objects with an imacro?  Or
>+    // will that be unsafe for some of our callers?

No need to ask and we avoid XXX comments now in favor of FIXME: bug NNNNNN comments. This one is completely fixable with imacrology, so please file a future bug.

>+    if (!v.isPrimitive())
>+        RETURN_STOP("can only inc primitives");

Uber-nit: "can inc primitives only".

Nice patch. bz, you need to do a JM patch too some time, get both decoder rings ;-).

/be
(In reply to comment #5)
> No need to ask and we avoid XXX comments now in favor of FIXME: bug NNNNNN

Done.  Bug 606071.

> Uber-nit: "can inc primitives only".

Done.

> Nice patch. bz, you need to do a JM patch too some time

I looked at the pic code some; got lost and gave up for the time being.  ;)
Blocks: 606071
Attachment #484715 - Attachment is obsolete: true
Attachment #484715 - Flags: approval2.0?
Attachment #487851 - Flags: approval2.0?
Robert, want to either approve or deny so I can stop worrying about merging this?  ;)
Attachment #487851 - Flags: approval2.0? → approval2.0+
Whiteboard: [need approval] → [need landing]
Pushed http://hg.mozilla.org/tracemonkey/rev/19f70f8c2b88
Flags: in-testsuite?
Whiteboard: [need landing] → fixed-in-tracemonkey
Blocks: 569327
http://hg.mozilla.org/mozilla-central/rev/19f70f8c2b88
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 613692
Depends on: 614782
Depends on: 617617
Target Milestone: --- → mozilla2.0b8
Depends on: 621374
No longer depends on: 613692
Depends on: 613692
You need to log in before you can comment on or make changes to this bug.