The default bug view has changed. See this FAQ.

IonMonkey: LCreateThis should accept constant operands

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: sstangl, Unassigned)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
Summary: IonMonkey: LCreateThis should not take operands → IonMonkey: LCreateThis should accept constant operands
(Reporter)

Comment 1

5 years ago
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.
Attachment #640458 - Flags: review?(dvander)
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.
Attachment #640458 - Flags: review?(dvander) → review+
(Reporter)

Comment 3

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/61bb4c14d50d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.