729278 - IonMonkey: Inline polymorphic calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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]]

(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]]

Attachment: Add InlineFunctionGuard instruction to IR.

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: Refactor inline call

Comment 4

[Kannan Vijayan [:djvj]]

Attachment: Add polymorphic inlining

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: Add InlineFunctionGuard instruction to IR.

Comment 6

[Kannan Vijayan [:djvj]]

Attachment: Refactor inline call

Comment 7

[Kannan Vijayan [:djvj]]

Attachment: Add polymorphic inlining

Comment 8

[Marty Rosenberg [:mjrosenb]]

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]]

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 10

[Kannan Vijayan [:djvj]]

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.

Comment 11

[Kannan Vijayan [:djvj]]

Attachment: 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]]

https://hg.mozilla.org/projects/ionmonkey/rev/6b71969a7cab