Last Comment Bug 772255 - IonMonkey: LCreateThis should accept constant operands
: IonMonkey: LCreateThis should accept constant operands
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks: IonSpeed 765980
  Show dependency treegraph
 
Reported: 2012-07-09 15:24 PDT by Sean Stangl [:sstangl]
Modified: 2012-07-09 18:08 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.83 KB, patch)
2012-07-09 17:45 PDT, Sean Stangl [:sstangl]
dvander: review+
Details | Diff | Review

Description Sean Stangl [:sstangl] 2012-07-09 15:24:01 PDT
LCreateThis currently takes operands for the callee and the prototype. This causes generated code to assume the shape below, for repetitive invocations of LCreateThis on the same function:

> LPointer X
> LPointer Y
> LCreateThis (X, Y) -> Z
> LPointer X
> LPointer Y
> LCreateThis (X, Y) -> Z
> LPointer X
> LPointer Y
> LCreateThis (X, Y) -> Z

These pointers are only necessary for the slow callVM path, and they must be compile-time constants.

We could save on these loads by using local, manual register allocation and movePtr() in visitCreateThisVMCall() after the call to saveLive(). GC can be appeased via ImmGCPtr.
Comment 1 Sean Stangl [:sstangl] 2012-07-09 17:45:49 PDT
Created attachment 640458 [details] [diff] [review]
patch

Permits LCreateThis to bake in constants rather than depend on LPointer. Minor performance gain, probably mattering more in artificially tight loops involving allocation.
Comment 2 David Anderson [:dvander] 2012-07-09 17:52:33 PDT
Comment on attachment 640458 [details] [diff] [review]
patch

Review of attachment 640458 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/CodeGenerator.cpp
@@ +1441,5 @@
> +    GeneralRegisterSet regs = GeneralRegisterSet::All();
> +    if (proto->isRegister() && regs.has(ToRegister(proto)))
> +        regs.take(ToRegister(proto));
> +    if (callee->isRegister() && regs.has(ToRegister(callee)))
> +        regs.take(ToRegister(callee));

Why wouldn't it have the proto/callee regs? (you could takeUnchecked if proto might == callee but that sounds impossible)

@@ +1447,5 @@
> +
> +    // Push arguments.
> +    if (proto->isConstant()) {
> +        masm.movePtr(ImmGCPtr(&proto->toConstant()->toObject()), scratch);
> +        pushArg(scratch);

I don't think you need a scratch register, you can pushArg() an ImmGCPtr.
Comment 3 Sean Stangl [:sstangl] 2012-07-09 18:08:29 PDT
http://hg.mozilla.org/projects/ionmonkey/rev/61bb4c14d50d

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