Closed
Bug 784568
Opened 12 years ago
Closed 12 years ago
IonMonkey: Simplify call paths
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sstangl, Unassigned)
Details
Attachments
(1 file)
29.66 KB,
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
The implementation of visitCallGeneric() is a huge, tangled mess. We can fix that: 1. Separate the hasSingleTarget() paths of visitCallGeneric() into a visitCallKnown() of some LCallKnown. 2. Attach a function pointer to each JSScript that is always Ion-callable. If no IonScript is attached, the pointer defaults to the equivalent of InvokeFunction() assuming an IonFramePrefix above it. This eliminates 3 movePtr()s and 1 branchPtr() from the hot path in the known-case; calling a function becomes at most 6 branch-free instructions, 3 of which construct the IonFramePrefix. Additionally, the entire InvokeFunction path disappears. This should also be more semantically pleasing: functions are always called in the same way regardless of compilation state. The generic case still must handle the ArgumentsRectifier, but that's only a single compare and branch on the hot path.
Reporter | ||
Comment 1•12 years ago
|
||
Cleans up calling code. Hopefully this addresses yesterday's valid complaints that visitCallGeneric() is an unholy mess of evolutionary code. Although this patch duplicates some functionality for the sake of sanity, it deletes more lines than it adds. This patch: - Factors common LCallFoo code into an LFormalCallInstructionHelper, for all LIR derived from MCall. - Adds an LCallKnown, so visitCallGeneric() doesn't have stupid branches all over the place. - Makes visitCall() readable. - Refactors visitCallGeneric() to finally only use a single safepoint.
Attachment #654396 -
Flags: review?(dvander)
Comment on attachment 654396 [details] [diff] [review] Sanitize calls. Review of attachment 654396 [details] [diff] [review]: ----------------------------------------------------------------- This indeed looks simpler. When we talked offline I was imagining something like: struct JSScript { .... ion::IonCode *invoke; Where |invoke| points to either a trampoline or ionScript->method()->raw(), which would remove a bunch of loads and a NULL check. I don't know how hard that is but it could simplify things even further. ::: js/src/ion/CodeGenerator.cpp @@ +737,5 @@ > + > + // Argument fixed needed. Load the ArgumentsRectifier. > + JS_ASSERT(ArgumentsRectifierReg != objreg); > + masm.bind(&thunk); > + masm.movePtr(ImmGCPtr(argumentsRectifier), objreg); // Necessary for GC marking. To make it clear this is a separate path, I like this pattern: masm.bind(&thunk); { .... } ::: js/src/ion/LIR-Common.h @@ +442,5 @@ > }; > > +// Common code for LIR descended from MCall. > +template <size_t Defs, size_t Operands, size_t Temps> > +class LFormalCallInstructionHelper : public LCallInstructionHelper<Defs, Operands, Temps> s/Formal/JS/
Attachment #654396 -
Flags: review?(dvander) → review+
Reporter | ||
Comment 3•12 years ago
|
||
(In reply to David Anderson [:dvander] from comment #2) > This indeed looks simpler. When we talked offline I was imagining something > like: > > struct JSScript { > .... > ion::IonCode *invoke; > > Where |invoke| points to either a trampoline or ionScript->method()->raw(), > which would remove a bunch of loads and a NULL check. I don't know how hard > that is but it could simplify things even further. Yeah, that's #2 on the list in Comment 0. It's way more complicated than this minor refactoring patch.
Reporter | ||
Comment 4•12 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/df9b62214347 I'll open a separate bug for part 2 in deference to policy.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 5•12 years ago
|
||
http://hg.mozilla.org/projects/ionmonkey/rev/ecf3af6113dd Fix templating issue on compilers other than my own.
You need to log in
before you can comment on or make changes to this bug.
Description
•