CacheIR: optimize volatile register spilling in IC code

RESOLVED FIXED in Firefox 54

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
Some IC stubs do a callWithABI to some helper function and save/restore volatile registers. Most platforms have a lot of double registers, so we spend a fair amount of time pushing/popping them. This is silly because (1) Baseline code doesn't have any live double registers so we don't have to save any of them and (2) Ion ICs have a LiveRegisterSet so we only need to save/restore the float registers from that set.

Adding a set of live float registers to the CacheIRCompiler base class should be easy and should be a measurable performance win in some cases.
(Assignee)

Updated

2 years ago
Blocks: 1259927
(Assignee)

Comment 1

2 years ago
Created attachment 8841916 [details] [diff] [review]
Patch

This improves the microbenchmark below from 152 ms to 119 ms so it seems worth it. With --no-ion I get 197 ms to 175 ms.

---

function f() {
    var a = [];
    for (var i=0; i<3456; i++)
        a.push(i);

    var s = "1234";
    var res = 0;
    var t = new Date;
    for (var i=0; i<10000000; i++) {
        res = a[s];
    }
    print(new Date - t);
    return res;

}
f();
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8841916 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8841916 [details] [diff] [review]
Patch

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

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +816,1 @@
>          LiveRegisterSet save(regs.asLiveSet());

nit: regs is only used as a way to init the LiveSet, remove "regs, and initialize the LiveRegisterSet accordingly.

@@ +817,5 @@
>  
>          masm.PushRegsInMask(save);
>  
>          masm.setupUnalignedABICall(scratch1);
>          masm.loadJSContext(scratch1);

existing-nit: Add a comment that NativeObject::growSlotsDontReportOOM does not GC.

@@ +1698,5 @@
>          break;
>      }
>  
> +    // Baseline doesn't allocate float registers so none of them are live.
> +    liveFloatRegs_.set() = FloatRegisterSet();

We should avoid manipulating the ".set()", as this is basically the internal state of the LiveSet, and I should remove this function.

I suggest you do the following instead:
  liveFloatRegs_ = LiveFloatRegisterSet(FloatRegisterSet());

::: js/src/jit/CacheIRCompiler.cpp
@@ +1494,5 @@
>      FailurePath* failure;
>      if (!addFailurePath(&failure))
>          return false;
>  
> +    AllocatableRegisterSet regs(GeneralRegisterSet::Volatile(), liveVolatileFloatRegs());

nit: regs is only used to initialize the LiveRegisterSet, which has the same constructors.

@@ +2131,5 @@
>  
>      Register obj = output.valueReg().scratchReg();
>      masm.unboxObject(output.valueReg(), obj);
>  
> +    AllocatableRegisterSet regs(GeneralRegisterSet::Volatile(), liveVolatileFloatRegs());

nit: same here.

@@ +2139,5 @@
>      masm.setupUnalignedABICall(scratch);
>      masm.loadJSContext(scratch);
>      masm.passABIArg(scratch);
>      masm.passABIArg(obj);
>      masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, WrapObjectPure));

existing-nit: add JS::AutoCheckCannotGC instance in the body of WrapObjectPure.

::: js/src/jit/CacheIRCompiler.h
@@ +568,5 @@
>  
>      MOZ_MUST_USE bool addFailurePath(FailurePath** failure);
>      MOZ_MUST_USE bool emitFailurePath(size_t i);
>  
> +    FloatRegisterSet liveVolatileFloatRegs() const {

nit: Add a comment that this function should only be used for non-GC calls made with callWithABI.

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +411,5 @@
>          MOZ_CRASH("Invalid cache");
>      }
>  
> +    if (liveRegs_)
> +        liveFloatRegs_.set() = liveRegs_->fpus();

nit: same here, do not use ".set()" to Mutate the internal state, prefer the copy constructor.

@@ +600,5 @@
>      masm.setupUnalignedABICall(scratch);
>      masm.movePtr(ImmGCPtr(atom), scratch);
>      masm.passABIArg(scratch);
>      masm.passABIArg(str);
>      masm.callWithABI(JS_FUNC_TO_DATA_PTR(void*, EqualStringsHelper));

existing-nit: add JS::AutoCheckCannotGC instance in the body of EqualStringsHelper.

@@ +1096,1 @@
>          LiveRegisterSet save(regs.asLiveSet());

existing-nit: Use LiveRegisterSet constructor instead of copying the content of the AllocatableRegisterSet.
Attachment #8841916 - Flags: review?(nicolas.b.pierron) → review+

Comment 3

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee6ba4fcb76f
Optimize volatile register spilling for C++ calls from IC stubs. r=nbp

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ee6ba4fcb76f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.