Closed Bug 1528270 Opened 11 months ago Closed 10 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: froydnj, Assigned: iain)

Details

(Keywords: perf)

Attachments

(1 file)

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.

(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!

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);

Keywords: perf
Priority: -- → P3

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

Assignee: nobody → iireland

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

Pushed by iireland@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69a09b505a35
Clean up pushCallArguments and pushSpreadCallArguments r=mgaudet
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.