Closed
Bug 728311
Opened 12 years ago
Closed 12 years ago
IonMonkey: IC for JSOP_BINDNAME
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
29.84 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
Adds an IC comparable to JM's BINDNAME IC. One difference is that JM has an inline path for the case where the scope chain is the global object (parent is NULL). Since getting the parent requires a few loads nowadays, this case has its own stub now.
Attachment #598925 -
Flags: review?(dvander)
Comment on attachment 598925 [details] [diff] [review] Patch Review of attachment 598925 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/IonCaches.cpp @@ +132,5 @@ > > Linker linker(masm); > IonCode *code = linker.newCode(cx); > + if (!code) > + return false; Whoops. Good catch. @@ +274,5 @@ > + > + // If the scope chain is a global object, guard that the parent > + // is NULL and move the scope chain into the output reg. > + > + // XXX: do we even need this guard? You can just guard that scopeChainReg() == scopeChain. @@ +334,5 @@ > + // Walk up the scope chain. > + while (tobj) { > + if (!IsCacheableNonGlobalScope(tobj)) { > + IonSpew(IonSpew_InlineCaches, "BINDNAME non-cacheable object"); > + failures.reset(); |failures.reset()| smells like an anti-pattern. What if we extracted out the IsCacheableNonGlobalScope + |tobj != obj| tests into a helper function? Then we could also avoid testing cacheability twice. ::: js/src/ion/IonCaches.h @@ +300,5 @@ > + } > + > + Register scopeChainReg() const { return u.bindname.scopeChain; } > + PropertyName *name() const { return u.bindname.name; } > + Register outputReg() const { return u.bindname.output; } super nit: Ion style is for these to be multi-line (this file strays a bit)
Attachment #598925 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 3•12 years ago
|
||
Addresses review comments + some more changes/refactoring to make the bindname IC look more like the getprop IC. Should hopefully also make it easier to add other *NAME IC's.
Attachment #598925 -
Attachment is obsolete: true
Attachment #600418 -
Flags: review?(dvander)
Comment on attachment 600418 [details] [diff] [review] Patch v2 Review of attachment 600418 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/CodeGenerator.cpp @@ +1913,5 @@ > +bool > +CodeGenerator::visitOutOfLineBindNameCache(OutOfLineCache *ool) > +{ > + LBindNameCache *ins = ool->cache()->toBindNameCache(); > + Register scopeChain = ToRegister(ins->getOperand(0)); Does this deserve a scopeChain() accessor? @@ +1917,5 @@ > + Register scopeChain = ToRegister(ins->getOperand(0)); > + Register output = ToRegister(ins->output()); > + > + RegisterSet liveRegs = ins->safepoint()->liveRegs(); > + liveRegs.maybeTake(output); Why is this maybeTake necessary?
Attachment #600418 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #4) > > Does this deserve a scopeChain() accessor? Done. Some instructions use these accessors and some don't; at some point we should probably decide when/where we want to use them - at least for the more complicated instructions having them may be a bit more readable. > > Why is this maybeTake necessary? It follows getprop, but now I'm pretty sure the live register set is guaranteed to never contain the output registers anyway (or we'd have seen other failures). So I removed the maybeTake from getprop and bindname. https://hg.mozilla.org/projects/ionmonkey/rev/0c780caf3eb6
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•