Last Comment Bug 729278 - IonMonkey: Inline polymorphic calls
: IonMonkey: Inline polymorphic calls
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Kannan Vijayan [:djvj]
:
Mentors:
Depends on:
Blocks: IonSpeed 768739 770309
  Show dependency treegraph
 
Reported: 2012-02-21 13:25 PST by Jan de Mooij [:jandem] (PTO until July 31)
Modified: 2012-07-05 08:59 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add InlineFunctionGuard instruction to IR. (10.18 KB, patch)
2012-06-23 14:16 PDT, Kannan Vijayan [:djvj]
no flags Details | Diff | Splinter Review
Refactor inline call (12.49 KB, patch)
2012-06-23 14:17 PDT, Kannan Vijayan [:djvj]
no flags Details | Diff | Splinter Review
Add polymorphic inlining (17.52 KB, patch)
2012-06-23 14:17 PDT, Kannan Vijayan [:djvj]
no flags Details | Diff | Splinter Review
Add InlineFunctionGuard instruction to IR. (10.25 KB, patch)
2012-06-25 12:15 PDT, Kannan Vijayan [:djvj]
marty.rosenberg: review+
Details | Diff | Splinter Review
Refactor inline call (12.56 KB, patch)
2012-06-25 12:16 PDT, Kannan Vijayan [:djvj]
marty.rosenberg: review+
Details | Diff | Splinter Review
Add polymorphic inlining (20.33 KB, patch)
2012-06-25 12:16 PDT, Kannan Vijayan [:djvj]
marty.rosenberg: review+
Details | Diff | Splinter Review
Fix constFun bug (2.15 KB, patch)
2012-07-04 14:25 PDT, Kannan Vijayan [:djvj]
jdemooij: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] (PTO until July 31) 2012-02-21 13:25:18 PST
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.
Comment 1 Jan de Mooij [:jandem] (PTO until July 31) 2012-02-23 06:38:16 PST
(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.
Comment 2 Kannan Vijayan [:djvj] 2012-06-23 14:16:42 PDT
Created attachment 636113 [details] [diff] [review]
Add InlineFunctionGuard instruction to IR.
Comment 3 Kannan Vijayan [:djvj] 2012-06-23 14:17:20 PDT
Created attachment 636114 [details] [diff] [review]
Refactor inline call
Comment 4 Kannan Vijayan [:djvj] 2012-06-23 14:17:40 PDT
Created attachment 636115 [details] [diff] [review]
Add polymorphic inlining
Comment 5 Kannan Vijayan [:djvj] 2012-06-25 12:15:05 PDT
Created attachment 636433 [details] [diff] [review]
Add InlineFunctionGuard instruction to IR.
Comment 6 Kannan Vijayan [:djvj] 2012-06-25 12:16:14 PDT
Created attachment 636435 [details] [diff] [review]
Refactor inline call
Comment 7 Kannan Vijayan [:djvj] 2012-06-25 12:16:55 PDT
Created attachment 636436 [details] [diff] [review]
Add polymorphic inlining
Comment 8 Marty Rosenberg [:mjrosenb] 2012-06-29 14:35:03 PDT
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?
Comment 9 Marty Rosenberg [:mjrosenb] 2012-07-02 17:49:00 PDT
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 &&
Comment 11 Kannan Vijayan [:djvj] 2012-07-04 14:25:49 PDT
Created attachment 639176 [details] [diff] [review]
Fix constFun bug

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.
Comment 12 Kannan Vijayan [:djvj] 2012-07-05 07:28:40 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/6b71969a7cab

Note You need to log in before you can comment on or make changes to this bug.