IonMonkey: Inline polymorphic calls

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: djvj)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 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

5 years ago
Assignee: jdemooij → kvijayan
(Assignee)

Comment 2

5 years ago
Created attachment 636113 [details] [diff] [review]
Add InlineFunctionGuard instruction to IR.
(Assignee)

Comment 3

5 years ago
Created attachment 636114 [details] [diff] [review]
Refactor inline call
(Assignee)

Comment 4

5 years ago
Created attachment 636115 [details] [diff] [review]
Add polymorphic inlining
(Assignee)

Comment 5

5 years ago
Created attachment 636433 [details] [diff] [review]
Add InlineFunctionGuard instruction to IR.
Attachment #636113 - Attachment is obsolete: true
Attachment #636433 - Flags: review?(mrosenberg)
(Assignee)

Comment 6

5 years ago
Created attachment 636435 [details] [diff] [review]
Refactor inline call
Attachment #636114 - Attachment is obsolete: true
Attachment #636435 - Flags: review?(mrosenberg)
(Assignee)

Comment 7

5 years ago
Created attachment 636436 [details] [diff] [review]
Add polymorphic inlining
Attachment #636115 - Attachment is obsolete: true
Attachment #636436 - Flags: review?(mrosenberg)
Blocks: 768739
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+
Blocks: 770309
Attachment #636435 - Flags: review?(mrosenberg) → review+
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

5 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

5 years ago
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.
Attachment #639176 - Flags: review?(jdemooij)
(Reporter)

Updated

5 years ago
Attachment #639176 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/projects/ionmonkey/rev/6b71969a7cab
(Assignee)

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.