1528270 - save a couple instructions in BaselineIC by folding shifts into address computations

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Nathan Froyd [:froydnj]]

Iain pointed out this bit in #jsapi:

https://searchfox.org/mozilla-central/rev/38035ee92463c9e9fbca729ac7e66476ac7eb27a/js/src/jit/BaselineIC.cpp#4063-4068

  // Push arguments: set up endReg to point to &array[argc]
  Register endReg = regs.takeAny();
  masm.movePtr(argcReg, endReg);
  static_assert(sizeof(Value) == 8, "Value must be 8 bytes");
  masm.lshiftPtr(Imm32(3), endReg);
  masm.addPtr(startReg, endReg);

which would be better expressed as something like (sorry, don't know the exact masm incantation):

  Register endReg = regs.takeAny();
  masm.mov(BaseValueIndex(startReg, argc), endReg);

so we'd save a few instructions on x86ish/ARMish processors.

There's a couple other instances of lshiftPtr in that file that look like they could benefit from the same treatment.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Nathan Froyd [:froydnj] from comment #0)

which would be better expressed as something like (sorry, don't know the exact masm incantation):

  Register endReg = regs.takeAny();
  masm.mov(BaseValueIndex(startReg, argc), endReg);

masm.computeEffectiveAddress :) Thanks for filing this!

Comment 2

[Iain Ireland [:iain]]

Similarly, there's no reason to emit two instructions here: https://searchfox.org/mozilla-central/source/js/src/jit/BaselineIC.cpp#3981-3987

// argPtr initially points to the last argument.
Register argPtr = regs.takeAny();
masm.moveStackPtrTo(argPtr);

// Skip 4 pointers pushed on top of the arguments: the frame descriptor,
// return address, old frame pointer and stub reg.
masm.addPtr(Imm32(STUB_FRAME_SIZE), argPtr);

Comment 3

[Iain Ireland [:iain]]

My WIP patch stack to convert call ICs to CacheIR includes a patch fixing these issues.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1528270: Clean up pushCallArguments and pushSpreadCallArguments r=mgaudet

This patch makes a couple of improvements to pushCallArguments, and one improvement to pushSpreadCallArguments.

1. The old code used two instructions to generate argPtr, where one instruction will do.
2. The old code jumped through some hoops to avoid adding |callee| and |this| to count before aligning the stack for jit frames. However, AlignJitStackBasedOnNArgs only cares whether nargs is even or odd. Since it doesn't make a difference whether we add 2 to nargs before or after alignment, this patch removes the "optimization".
3. The previous version of pushSpreadCallArguments used three instructions to find the end of the argument array, where one computeEffectiveAddress would do.

Depends on D22778

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69a09b505a35
Clean up pushCallArguments and pushSpreadCallArguments r=mgaudet

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/69a09b505a35