Closed
Bug 1342856
Opened 8 years ago
Closed 8 years ago
CacheIR: optimize volatile register spilling in IC code
Categories
(Core :: JavaScript Engine: JIT, defect, P1)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
8.16 KB,
patch
|
nbp
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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+
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•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•