Closed
Bug 601657
Opened 14 years ago
Closed 14 years ago
JM: Remove FrameState::syncData() hack
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: dvander, Assigned: sstangl)
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
17.33 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
FrameState::syncData() has a hack where it syncs types if the payload was constant. Every place that uses syncData() has to then do a constant-check and avoid syncType(). The order of data/type syncing is also implicitly mandated. No one seems to really know why we do this. Sean suspects something to do with double constants, since historically we've treated their type differently. It also might have to do with earlier unsound optimizations to avoid syncing the payload of |undefined|. The latter no longer exists. For the former, we should audit the use of getKnownType in FrameState with regards to syncing, and get rid of this hack.
Assignee | ||
Updated•14 years ago
|
Assignee: general → sstangl
Assignee | ||
Comment 1•14 years ago
|
||
Redefine getPayload(), getTypeTag() to be shared between 32-bit and 64-bit. Redefine syncData() and syncType() to eliminate the constant-saving hack in syncData(). Refactor some code that can be expressed more simply as a result.
Attachment #480845 -
Flags: review?(dvander)
Reporter | ||
Comment 2•14 years ago
|
||
Comment on attachment 480845 [details] [diff] [review] Eliminate syncData() constant hack. Yay! >- masm.move(Imm64(known->getKnownShiftedTag()), maskReg); >+ masm.move(Imm64(known->getKnownTag()), maskReg); Should this be ImmType, for clarity? BTW, the reason for the mysterious "if (closed) .. forgetEntry" is that forgetEntry makes sure to remove any copies and free associated registers. resetSynced alone won't do that.
Attachment #480845 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 3•14 years ago
|
||
ImmTag, yeah. Usage of ImmTag is very much discouraged, as it opens boxes into the implementation of the type system -- but we're doing that anyway in that location, so might as well. Also fixed a register clobber bug for x64 in jsop_stricteq(). Great. http://hg.mozilla.org/tracemonkey/rev/c3bd2594777a
Whiteboard: fixed-in-tracemonkey
Reporter | ||
Comment 4•14 years ago
|
||
There was no clobber bug there, frame.allocReg() isn't pinned to an FE so it is impossible to evict. I'm surprised this change didn't assert trying to pin an owned register.
Comment 5•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/c3bd2594777a
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•