Closed
Bug 729278
Opened 12 years ago
Closed 12 years ago
IonMonkey: Inline polymorphic calls
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: djvj)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 3 obsolete files)
10.25 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
12.56 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
20.33 KB,
patch
|
mjrosenb
:
review+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
JM+TI can inline up to 5 different functions at a single call site. IM needs to do this too for v8-richards and v8-deltablue. We already inline monomorphic calls, so this shouldn't be too hard.
Reporter | ||
Comment 1•12 years ago
|
||
(In reply to Jan de Mooij (:jandem) from comment #0) > IM needs to do this too for v8-richards and v8-deltablue. v8-richards does not need this, it turned out to be bug 729920.
Assignee | ||
Updated•12 years ago
|
Assignee: jdemooij → kvijayan
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Assignee | ||
Comment 5•12 years ago
|
||
Attachment #636113 -
Attachment is obsolete: true
Attachment #636433 -
Flags: review?(mrosenberg)
Assignee | ||
Comment 6•12 years ago
|
||
Attachment #636114 -
Attachment is obsolete: true
Attachment #636435 -
Flags: review?(mrosenberg)
Assignee | ||
Comment 7•12 years ago
|
||
Attachment #636115 -
Attachment is obsolete: true
Attachment #636436 -
Flags: review?(mrosenberg)
Comment 8•12 years ago
|
||
Comment on attachment 636433 [details] [diff] [review] Add InlineFunctionGuard instruction to IR. Review of attachment 636433 [details] [diff] [review]: ----------------------------------------------------------------- just a couple of nits. ::: js/src/ion/CodeGenerator.cpp @@ +195,5 @@ > + > +bool > +CodeGenerator::visitInlineFunctionGuard(LInlineFunctionGuard *lir) > +{ > + const Register reg = ToRegister(lir->input()); Nit: can you call this something more descriptive than "reg", like inputReg? ::: js/src/ion/Lowering.cpp @@ +331,5 @@ > + ins->functionBlock(), > + ins->fallbackBlock()); > + return add(lir); > +} > + there is an awful lot of whitespace here (initially each of the arguments wrapped). Could you put the new on a new line so the indentation is a bit more reasonable?
Attachment #636433 -
Flags: review?(mrosenberg) → review+
Updated•12 years ago
|
Attachment #636435 -
Flags: review?(mrosenberg) → review+
Comment 9•12 years ago
|
||
Comment on attachment 636436 [details] [diff] [review] Add polymorphic inlining Review of attachment 636436 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. We might want to consider inlining cases that are combinations of inlineable natives and scripted calls, but that can wait ::: js/src/ion/IonBuilder.cpp @@ +165,5 @@ > > return obj->toFunction(); > } > > +unsigned Personally, I am not a fan of the type "unsigned", and like uint32_t better @@ +195,5 @@ > + cx->compartment->types.compiledInfo), false); > + } > + * > + */ > + Nuke that before committing, particularly since it looks like it won't compile. @@ +2799,5 @@ > { > + // polymorphic = 0 means not polymorphic. > + // polymorphic = 1 means a guarded polymorphic case. > + // polymorphic = 2 means a un-guarded polymorphic case (i.e. final case). > + That should probably be an enum rather than an int, and these comments should go with the enum definition. Finally, based on looking at the code briefly, it is not immediately evident what the difference between cases 0 and 2 are. You may wish to document that as well. @@ +3247,5 @@ > // Acquire known call target if existent. > + AutoObjectVector targets(cx); > + unsigned numTargets = getPolyCallTargets(argc, pc, targets, 4); > + > + IonSpew(IonSpew_Inlining, "KVKV jsop_call numTargets=%u", numTargets); You should remove the KVKV and probably add in some more words to make it closer to a proper sentence. @@ +3270,5 @@ > + // Cannot inline construction (yet). > + if (!constructing && numTargets > 0) { > + if (makeInliningDecision(targets)) > + return inlineScriptedCall(targets, argc); > + } There have been a couple of changes to this code, make sure your merge gets all of them. Also, rather than nested ifs, you should join them with &&
Attachment #636436 -
Flags: review?(mrosenberg) → review+
Assignee | ||
Comment 10•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/51c5ea99a47e https://hg.mozilla.org/projects/ionmonkey/rev/54558ba6a5d6 https://hg.mozilla.org/projects/ionmonkey/rev/8523fd225c0d https://hg.mozilla.org/projects/ionmonkey/rev/c83f8157e50d https://hg.mozilla.org/projects/ionmonkey/rev/5d9e10b2f586 https://hg.mozilla.org/projects/ionmonkey/rev/45315f6ccb19 https://hg.mozilla.org/projects/ionmonkey/rev/9cf3ea112635 https://hg.mozilla.org/projects/ionmonkey/rev/a4dfbc165f15 *phew* That took a bit of massaging. One more fix-up patch on the way for a bug that jandem discovered before closing this bug.
Assignee | ||
Comment 11•12 years ago
|
||
Jan, this fixes the constFun bug you brought up on IRC. I'm pretty sure this is the right way to fix it. Just wanted a second opinion.
Attachment #639176 -
Flags: review?(jdemooij)
Reporter | ||
Updated•12 years ago
|
Attachment #639176 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 12•12 years ago
|
||
https://hg.mozilla.org/projects/ionmonkey/rev/6b71969a7cab
Assignee | ||
Updated•12 years ago
|
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
•