Closed Bug 729278 Opened 12 years ago Closed 12 years ago

IonMonkey: Inline polymorphic calls

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: djvj)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 3 obsolete files)

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.
(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: jdemooij → kvijayan
Attached patch Refactor inline call (obsolete) — Splinter Review
Attached patch Add polymorphic inlining (obsolete) — Splinter Review
Attachment #636113 - Attachment is obsolete: true
Attachment #636433 - Flags: review?(mrosenberg)
Attachment #636114 - Attachment is obsolete: true
Attachment #636435 - Flags: review?(mrosenberg)
Attachment #636115 - Attachment is obsolete: true
Attachment #636436 - Flags: review?(mrosenberg)
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+
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+
Attached patch Fix constFun bugSplinter Review
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)
Attachment #639176 - Flags: review?(jdemooij) → review+
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.