Closed Bug 1322093 Opened 3 years ago Closed 3 years ago

Generate Ion IC stubs from CacheIR

Categories

(Core :: JavaScript Engine: JIT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 2 open bugs)

Details

Attachments

(22 files, 2 obsolete files)

118.40 KB, patch
Details | Diff | Splinter Review
71.44 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
5.10 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
5.34 KB, patch
nbp
: review+
Details | Diff | Splinter Review
8.71 KB, patch
nbp
: review+
Details | Diff | Splinter Review
7.56 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
13.39 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
6.72 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
4.72 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
7.59 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
2.34 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
4.06 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
16.47 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
47.75 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
6.50 KB, patch
nbp
: review+
Details | Diff | Splinter Review
44.29 KB, patch
nbp
: review+
Details | Diff | Splinter Review
46.70 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
1.28 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
94.40 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
632 bytes, patch
h4writer
: review+
Details | Diff | Splinter Review
81.32 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
1.01 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
Once bug 1322091 is fixed, we should be able to add IonCacheIRCompiler (similar to BaselineCacheIRCompiler), to generate Ion GetProperty IC stubs from the same CacheIR that's used for Baseline.

This has a lot of benefits, most importantly:

* Removes a lot of code from IonCaches.cpp that's duplicating logic that exists elsewhere for Baseline. CacheIR.cpp will be the single source of truth to determine if/how to optimize a property access.

* As a result, there should no longer be stuff like (we currently see all of these):
  - Ion has an IC stub for something but Baseline doesn't.
  - Baseline has ICs for something and IonBuilder has fast paths but Ion lacks IC support to fall back on.
  - Both Baseline and Ion have an IC stub for something but one of them handles more cases than the other.

* We should be able to reuse CacheRegisterAllocator we use for CacheIR Baseline IC stubs. This will get rid of a lot of tricky, manual register management in Ion ICs.

There are definitely some challenges here though: Ion IC register allocation is more complicated, Ion ICs can have type-specialized input/output registers, etc. So far I'm not aware of any serious showstoppers though.
Two other issues:

* As I mentioned in comment 0, Ion handles more cases than Baseline (GETELEM accessing holes for instance). We should check for such cases and add them to CacheIR (this will also improve our Baseline IC coverage!).

* Ion checks cache.monitoredResult() or output.hasValue() in a few cases and if that's false we don't attach a stub. I hope we can get rid of these checks though or do something better.
Priority: -- → P1
My initial plan (comment 0) was to add IonCacheIRCompiler and then plug it into Ion's GetPropertyIC, but this doesn't fix some of the issues we have with Ion ICs (being unable to unlink/inspect individual stubs, requiring W^X toggling when attaching stubs, etc). After thinking about it more, I think there's a nicer design I want to investigate first, that's more like Baseline stubs. That might also let us share even more code.
Last Friday and this morning I worked on a prototype for the approach described in comment 2.

This adds an IonGetPropertyIC based on CacheIR. It can run all of Octane/Sunspider/Kraken on x64, except for the getter stuff I temporarily disabled in CacheIR (and in IonCaches.cpp to get a fair comparison). Supporting getters and other GC calls in the Ion IC compiler shouldn't be terribly difficult but it will require some careful work.

The new Ion IC system is a bit of a cross between IonCaches and BaselineIC. We emit an indirect jump to enter the IC (and also when falling through to the next stub). This is nice because it avoids the need to do code patching and W^X toggling (which can be expensive, we have a perf bug on file for this) when purging stubs, attaching a new stub, etc.

Other than that, the native code we emit should be more-or-less similar to the current IonCaches code. I checked some common cases and the register allocator does a fine job, very similar to the manual regalloc we do now. Later on we can also let the register allocator use registers that aren't live.

Anyway, on Octane/Sunspider/Kraken, performance is comparable to IonCaches performance \o/. I'm very happy with this - ICs are very important for several Octane tests (disabling optimized stubs is a massive slowdown) but we're keeping up nicely.

Again, this patch is just a quick-and-dirty hack: it duplicates code that we should share and leaks memory like a sieve, but now I have a much better idea what code changes are required and the next step is rewriting this as a series of small patches.
One thing I noticed while working on this is that Ion's TypedArray-length stub is broken. Baseline had a similar one and it got removed at some point, when we made this property a getter. For Ion nobody noticed because we will try to attach a getter call stub first so we don't get to tryAttachTypedArrayLength :)
That sounds like bug 1027846, maybe?  I haven't been able to scrape up the time to try to figure out some convoluted scheme to have multiple freezes or something to let us presume the normal length accessor property is in place, to just do the field lookup inline ourselves.

That said, I can't see why this one weird place is so important that it can't use optimization tactics used by all inherited properties, and I'd remove that gunk in a heartbeat if shu weren't grumpy about it.
Blocks: 1323099
This moves code that isn't Baseline-specific from BaselineCacheIR.cpp/h to CacheIRCompiler.cpp/h, so we can use it for Ion.

I moved some larger functions out-of-line but other than that it's just moving code. I considered using |hg cp|, but we're moving code to multiple files so it's a bit tricky to get right.
Attachment #8818879 - Flags: review?(hv1989)
This patch adds a list of ops that are defined in CacheIRCompiler and shared between Baseline/Ion ICs. I added some ops just to show how this works, but there are more ops we can do this for later.

(It's probably possible to do something similar with |virtual|, but I want to avoid virtual calls and this has minimal boilerplate.)
Attachment #8818886 - Flags: review?(hv1989)
Attachment #8818886 - Attachment is obsolete: true
Attachment #8818886 - Flags: review?(hv1989)
Attachment #8818888 - Flags: review?(hv1989)
This adds CacheRegisterAllocator::knownType to get the JSValueType for an operand (JSVAL_TYPE_UNKNOWN if we don't know the type).

We can then use this to optimize away GuardIsObject et al if we know these instructions always succeed. For Baseline this doesn't matter because it has Value operands, but for Ion this will be very effective.
Attachment #8818913 - Flags: review?(nicolas.b.pierron)
Ion ICs can have ConstantOrRegister operands. This patch adds OperandLocation::Constant to handle the constant case.

Knowing the constant Value will also make it easy to NOP GuardSpecificAtom and GuardSpecificSymbol later.
Attachment #8818925 - Flags: review?(nicolas.b.pierron)
Attachment #8818913 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8818888 [details] [diff] [review]
Part 2 - Share Baseline/Ion codegen

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

Nice
Attachment #8818888 - Flags: review?(hv1989) → review+
Comment on attachment 8818879 [details] [diff] [review]
Part 1 - Split BaselineCacheIR.cpp/h

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

Ŵe should rename BaselineCacheIR.cpp to BaselineCacheIRCompiler.cpp to make the relation between the CacheIRCompiler.cpp file and BaselineCacheIR.cpp more clear.
Same for IonCacheIR.cpp off course
Attachment #8818879 - Flags: review?(hv1989) → review+
For the failure paths, we have some code to restore the IC input operands. This patch factors that out into CacheRegisterAllocator::restoreInputState so we can later use it in more places.
Attachment #8818948 - Flags: review?(hv1989)
This patch adds some helper functions to avoid code duplication. It's necessary for the next patch, but also a nice cleanup on its own.
Attachment #8818949 - Flags: review?(hv1989)
restoreInputState currently only knows how to restore Value inputs (as that's what Baseline uses), this patch builds on the previous patches and improves restoreInputState so it knows how to restore typed registers. We will need this for Ion.
Attachment #8818950 - Flags: review?(hv1989)
Renames BaselineCacheIR.* to BaselineCacheIRCompiler.* as suggested. It's pretty long but I agree it's more consistent.
Attachment #8818959 - Flags: review?(hv1989)
This patch removes GetPropIRGenerator::res_ as it's unused. This code runs before we do the GetProperty fallback code that sets |res|, so it makes no sense to pass it. Not sure what I was thinking.

I also added some asserts to check writer.failed() is false, to make sure the callers check this properly.
Attachment #8818961 - Flags: review?(evilpies)
Attachment #8818925 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8818961 - Flags: review?(evilpies) → review+
This adds overloads of CacheRegisterAllocator::initInputLocation for ConstantOrRegister, TypedOrValueRegister, etc so we can connect the Ion ICs to the register allocator.
Attachment #8818965 - Flags: review?(evilpies)
Attachment #8818965 - Flags: review?(evilpies) → review+
Ion supports idempotent GetProperty ICs. This patch adds GetPropIRGenerator::tryAttachIdempotentStub for this case.

The current Ion IC also allows array length stubs if idempotent, but that requires a lot of complexity: passing the list of locations to GetPropertyIC::allowArrayLength, etc. Looking at |hg blame|, this was not added to fix an actual perf bug, just "nice to have" and then fuzz bugs turned up that required all this additional complexity. My prototype patch was fast enough without this, so let's not support that until we know we need it.
Attachment #8818995 - Flags: review?(evilpies)
Forgot to qref.
Attachment #8818995 - Attachment is obsolete: true
Attachment #8818995 - Flags: review?(evilpies)
Attachment #8818996 - Flags: review?(evilpies)
Comment on attachment 8818996 [details] [diff] [review]
Part 11 - GetPropIRGenerator changes for idempotent ICs

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

I checked some of the Ion code for idempotent ICs, because I am really not that familiar with this behavior. It seems kind of magic/random. Ion has code for invalidating idempotent ICs do we need that, too?

::: js/src/jit/CacheIR.cpp
@@ +49,5 @@
>  GetPropIRGenerator::tryAttachStub()
>  {
> +    // pc_ should only be null for idempotent ICs, and idempotent ICs should
> +    // call tryAttachIdempotentStub instead.
> +    MOZ_ASSERT(pc_);

I think it would be nicer to have an explicit idempotent flag.

@@ +133,5 @@
> +GetPropIRGenerator::tryAttachIdempotentStub()
> +{
> +    // For idempotent ICs, only attach stubs for plain data properties.
> +    // This ensures (1) the lookup has no side-effects and (2) Ion has complete
> +    // static type information and we don't have to monitor the result. Because

We do type monitor right, because I don't see any change to EmitReadSlotReturn?

@@ +161,5 @@
>  
> +    if (!pc) {
> +        // This is an idempotent IC, don't attach a missing-property stub.
> +        // See tryAttachStub.
> +        return false;

We could have just checked this in tryAttachNative instead of asserting on holder.
(In reply to Tom Schuster [:evilpie] from comment #21)
> Ion has code for invalidating idempotent ICs do we need that, too?

It will be added by a later patch that adds the IC code, but it's pretty straight-forward.

> I think it would be nicer to have an explicit idempotent flag.

You mean a flag passed to tryAttachStub or the constructor? That's what I had at first :)

> We do type monitor right, because I don't see any change to
> EmitReadSlotReturn?

Right, but type monitoring is basically a nop for Ion ICs - IonBuilder inserts MTypeBarrier if needed.

> We could have just checked this in tryAttachNative instead of asserting on
> holder.

That's what I did initially, but after this we check *pc == JSOP_GETXPROP and that will crash if pc is nullptr...
(In reply to Jan de Mooij [:jandem] from comment #22)
> (In reply to Tom Schuster [:evilpie] from comment #21)
> > Ion has code for invalidating idempotent ICs do we need that, too?
> 
> It will be added by a later patch that adds the IC code, but it's pretty
> straight-forward.
> 
> > I think it would be nicer to have an explicit idempotent flag.
> 
> You mean a flag passed to tryAttachStub or the constructor? That's what I
> had at first :)
> 
I was thinking the constructor, it is not strictly needed the 4 uses are sufficiently commented.
> > We do type monitor right, because I don't see any change to
> > EmitReadSlotReturn?
> 
> Right, but type monitoring is basically a nop for Ion ICs - IonBuilder
Okay, I believe you :)
> inserts MTypeBarrier if needed.
> 
> > We could have just checked this in tryAttachNative instead of asserting on
> > holder.
> 
> That's what I did initially, but after this we check *pc == JSOP_GETXPROP
> and that will crash if pc is nullptr...
Ups, yeah :)
Attachment #8818996 - Flags: review?(evilpies) → review+
A lot of ops can be shared between Baseline/Ion without any code changes \o/. These are ops that don't use the stub data and the output register.

The next patch will do something similar for ops that use the output register.
Attachment #8819553 - Flags: review?(evilpies)
(1) This patch adds a Maybe<TypedOrValueRegister> to CacheIRCompiler to store the IC's output register. This then lets us share codegen for a lot more ops between Baseline and Ion.

(2) There used to be a footgun with the output register: it can alias other registers, so ops could only write to it after doing all guards etc. The patch fixes this and some related Ion-specific subtleties - it adds an AutoOutputRegister class that allocates the output register as a fixed scratch. This ensures the output register is writable and doesn't alias anything.

I spent a lot of time thinking about this the past days, as it may make regalloc a bit harder, but I think it's the right thing to do: one of my goals for CacheIR is to make it really easy to add more ops without having to know complicated regalloc constraints, and this definitely helps with that.

(3) Because this can result in more register pressure, I added AutoScratchRegisterMaybeOutput that's like AutoScratchRegister but it can reuse the output's scratch register (if there is one).

(4) For Ion, because we attach stubs before doing type monitoring and invalidation, it's possible the output register type does not match what we expect (for instance it could be int32-typed in LoadUndefinedResult). This patch adds some checks for this and emits masm.assumeUnreachable to ensure these cases don't actually show up at runtime.

If this turns out to be annoying, we could fix this later by generating CacheIR before the GetProperty/TypeMonitor call and then *compiling* the stub after that. For now this is sufficient though.

(5) This patch also optimizes fixed register allocation a bit: if there's a register available we use that instead of spilling to the stack. This helps (2) a lot and is basically bug 1317292.

---

Sorry, I realize this patch moves code and then changes it and could be split up more, but I already spent a lot of time splitting things up and I think this patch isn't *too* bad.

I'm pretty happy with how this turned out. We will share codegen for most ops with Baseline, and AutoOutputRegister simplifies things.
Attachment #8819554 - Flags: review?(hv1989)
Comment on attachment 8819553 [details] [diff] [review]
Part 12 - Move ops that are trivial to share

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

\o/ After the WIP patch I was concerned we would have to duplicate a lot of code, I am glad we can share so much here + the other patch. This makes it even easier to introduce new ICs and reduces maintenance cost.

::: js/src/jit/CacheIRCompiler.h
@@ +27,5 @@
> +    _(GuardNoDetachedTypedObjects)        \
> +    _(GuardNoDenseElements)               \
> +    _(LoadProto)                          \
> +    _(LoadDOMExpandoValue)                \
> +    _(GuardType)

Nit: GuardType should be after GuardIsSymbol.
Attachment #8819553 - Flags: review?(evilpies) → review+
In a later patch I need to (sometimes) store live regs to the stack after reserving space for it and pushing values on top of that. This patch adds masm.storeRegsInMask to do this.
Attachment #8819927 - Flags: review?(nicolas.b.pierron)
IonCacheIRCompiler will support callVM. To implement that, I need to push a frame similar to IonAccessorIC frames. This patch renames IonAccessorIC to IonICCall.
Attachment #8819938 - Flags: review?(nicolas.b.pierron)
This patch adds an IonGetPropertyIC that only has a fallback path. For the IonIC things I used "IC" instead of the current "Cache". When we remove IonCaches.cpp, we can remove the methods that use Cache and eliminate the duplication.

As I mentioned before, main difference is that this uses indirect jumps and it's now possible to iterate/unlink Ion IC stubs.

I also added an EnableIonCacheIR flag that's currently set to |false|. We can flip that to true after landing all patches in this bug.
Attachment #8819960 - Flags: review?(hv1989)
Ion's GetProperty IC currently doesn't support typed objects, but we do emit CacheIR for this (because Baseline had a stub for it).

This patch ensures we add a type barrier in IonBuilder when the IC may read from a typed object.
Attachment #8819963 - Flags: review?(bhackett1024)
This adds IonCacheIRCompiler. Note that we now support callVM in Ion ICs, but I decided to emit the same code as the current IonCache for CallNativeGetterResult, CallProxyGetResult etc to avoid performance regressions.
Attachment #8819992 - Flags: review?(hv1989)
A one-liner to enable the new code.
Attachment #8819994 - Flags: review?(hv1989)
8 files changed, 11 insertions(+), 1951 deletions(-)

There's more code to remove once we convert NameIC (these ICs share code).
Attachment #8819997 - Flags: review?(hv1989)
Comment on attachment 8819963 [details] [diff] [review]
Part 17 - Barrier TypedObject reads

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

I guess I don't understand why this patch is necessary.  Typed objects have well defined property layouts and if we know the set of typed object groups being read from then we should be able to determine whether a barrier is necessary by looking at the type information on those groups, just like with normal native objects.

Additionally, PropertyReadNeedsTypeBarrier is used by all reads that IonBuilder does, so won't this cause all of those reads to have a type barrier as well?
(In reply to Brian Hackett (:bhackett) from comment #34)
> Typed objects have
> well defined property layouts and if we know the set of typed object groups
> being read from then we should be able to determine whether a barrier is
> necessary by looking at the type information on those groups, just like with
> normal native objects.

Typed objects are still Nightly-only and I don't think there's much short-term interest in changing that, and the patch is actually improving things for typed objects because we will now have an Ion IC for them. Mind if I file a follow-up bug for improving this? I'm not that familiar with typed objects and this bug has a lot of changes already.

> Additionally, PropertyReadNeedsTypeBarrier is used by all reads that
> IonBuilder does, so won't this cause all of those reads to have a type
> barrier as well?

When we read scalars for instance (pushScalarLoadFromTypedObject) we don't call pushTypeBarrier, so we shouldn't add a barrier.
Comment on attachment 8819963 [details] [diff] [review]
Part 17 - Barrier TypedObject reads

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

OK, sounds good.
Attachment #8819963 - Flags: review?(bhackett1024) → review+
Comment on attachment 8818948 [details] [diff] [review]
Part 5 - Factor out restoreInputState

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

Looking through the code I saw that "addFailurePath" needs to have MOZ_MUST_USE

::: js/src/jit/CacheIRCompiler.cpp
@@ +617,5 @@
>  
>  void
>  CacheIRCompiler::emitFailurePath(size_t i)
>  {
>      FailurePath& failure = failurePaths[i];

Please use another name for "i" here or for the loop under this.

@@ +629,5 @@
> +    allocator.restoreInputState(masm);
> +}
> +
> +void
> +CacheRegisterAllocator::restoreInputState(MacroAssembler& masm)

Maybe move to were all other "CacheRegisterAllocator" functions are, instead of in between the CacheIRCompiler functions
Attachment #8818948 - Flags: review?(hv1989) → review+
Attachment #8819927 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8819938 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8818949 [details] [diff] [review]
Part 6 - Add helpers for pushing/popping operands

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

Nice cleanup!

If I understand correctly if we call popValue (in the non-top location):
We copy it do registers and overwrite that this is located in registers now.
But we do not free the stack value (since else we need to copy everything that is after that stack location).
I assume this can lead to quite bad behavior since we don't know that "stack" position is now free.
As a result having stack positions with holes that contain no information anymore?

Should we keep these "empty/unused" spaces in a list that during spilling will get reused again?
Instead of defaulting to pushing to the end of the stack?

=> I assume this would be followup bug if deemed important enough.

::: js/src/jit/CacheIRCompiler.cpp
@@ -23,5 @@
>  
>        case OperandLocation::ValueStack: {
> -        // The Value is on the stack. If it's on top of the stack, unbox and
> -        // then pop it. If we need the registers later, we can always spill
> -        // back. If it's not on the top of the stack, just unbox.

Shouldn't this comment get added again in popValue?

@@ -78,5 @@
>        }
>  
>        case OperandLocation::PayloadStack: {
> -        // The payload is on the stack. If it's on top of the stack we can just
> -        // pop it, else we emit a load.

ditto.
Attachment #8818949 - Flags: review?(hv1989) → review+
Comment on attachment 8818950 [details] [diff] [review]
Part 7 - Make restoreInputState work for Ion

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

::: js/src/jit/CacheIRCompiler.cpp
@@ +682,5 @@
>      size_t numInputOperands = origInputLocations_.length();
>      MOZ_ASSERT(writer_.numInputOperands() == numInputOperands);
>  
>      for (size_t j = 0; j < numInputOperands; j++) {
>          OperandLocation orig = origInputLocation(j);

Feel free to ignore. But can we rename orig -> dest? I made it easier to read the code beneath. I know that orig is also correct. But reading "orig.kind" makes it easy to think that is the source ;).

@@ +723,5 @@
> +        } else if (orig.kind() == OperandLocation::PayloadReg) {
> +            // We have to restore a payload register.
> +            switch (cur.kind()) {
> +              case OperandLocation::ValueReg:
> +                MOZ_ASSERT(orig.payloadType() != JSVAL_TYPE_DOUBLE);

can you add debug asserts that cur.valueReg()->typeReg() == orig.payloadType()

@@ +727,5 @@
> +                MOZ_ASSERT(orig.payloadType() != JSVAL_TYPE_DOUBLE);
> +                masm.unboxNonDouble(cur.valueReg(), orig.payloadReg());
> +                continue;
> +              case OperandLocation::PayloadReg:
> +                masm.mov(cur.payloadReg(), orig.payloadReg());

Can you assert here that "cur.payloadType() == orig.payloadReg()"

@@ +730,5 @@
> +              case OperandLocation::PayloadReg:
> +                masm.mov(cur.payloadReg(), orig.payloadReg());
> +                continue;
> +              case OperandLocation::PayloadStack: {
> +                popPayload(masm, &cur, orig.payloadReg());

Can you assert here that "cur.payloadType() == orig.payloadReg()"

@@ +736,5 @@
> +              }
> +              case OperandLocation::ValueStack:
> +                MOZ_ASSERT(stackPushed_ >= sizeof(js::Value));
> +                MOZ_ASSERT(cur.valueStack() <= stackPushed_);
> +                MOZ_ASSERT(orig.payloadType() != JSVAL_TYPE_DOUBLE);

can you add debug asserts that the type on the stack corresponds with orig.payloadType()
Attachment #8818950 - Flags: review?(hv1989) → review+
Comment on attachment 8818959 [details] [diff] [review]
Part 8 - Rename BaselineCacheIR.*

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

Thanks!
Attachment #8818959 - Flags: review?(hv1989) → review+
Comment on attachment 8819554 [details] [diff] [review]
Part 13 - Share ops that use the output register

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

First a suggestion: can we rename emitXXXResult to emitXXXToOutput() 

Secondly this still leaves a door open for issues since we don't know how to spill the output.
AutoOutputRegister();
AutoScratchRegister(R0);
If I understand this will always assert.
Can you double check that? (That we always asserts when we are spilling in order to get the output registers?)

The biggest issue I see is how AutoOutputRegister is implemented. That class should only contain logic to
allocate the output register(s). It seems like now it has been written to be always used with AutoScratchRegisterMaybeOutput.
As a result some name bleeding is happening to AutoOutputRegister. "Scratch register" has nothing to do with AutoOutputRegister.
And it think that should be fixed. Therefore I made comment on how to remove all those.

::: js/src/jit/CacheIRCompiler.cpp
@@ +270,5 @@
> +    allocateFixedRegister(masm, reg.typeReg());
> +#else
> +    allocateFixedRegister(masm, reg.valueReg());
> +#endif
> +}

I don't think is used now (with my comment to remove it in AutoOutputRegister).
As a result this can get removed.

@@ +872,5 @@
> +    if (output_.hasValue())
> +        valueScratch_.emplace(compiler.allocator, compiler.masm, output_.valueReg());
> +    else if (!output_.typedReg().isFloat())
> +        typedScratch_.emplace(compiler.allocator, compiler.masm, output_.typedReg().gpr());
> +}

Can you find a better place to put this. I.e. not between the emitXXX functions?

::: js/src/jit/CacheIRCompiler.h
@@ +335,5 @@
> +    ~AutoValueScratchRegister() {
> +        alloc_.releaseValueRegister(reg_);
> +    }
> +    operator ValueOperand() const { return reg_; }
> +};

I don't think is used now (with my comment to remove it in AutoOutputRegister).
As a result this can get removed.

@@ +446,5 @@
> +class MOZ_RAII AutoOutputRegister
> +{
> +    TypedOrValueRegister output_;
> +    mozilla::Maybe<AutoScratchRegister> typedScratch_;
> +    mozilla::Maybe<AutoValueScratchRegister> valueScratch_;

IMHO: The abstraction to use AutoScratchRegister and AutoValueScratchRegister don't help readability here. "Scratch register" has nothing to do with "AutoOutputRegister". Which makes it harder trying to read this code for understanding. I think it would be better if the constructor and destructor call the alloc.allocateFixedRegister and alloc.releaseRegister directly and remove these two Maybe<AutoXXXX>.

@@ +460,5 @@
> +            return output_.valueReg().scratchReg();
> +        if (!output_.typedReg().isFloat())
> +            return output_.typedReg().gpr();
> +        return InvalidReg;
> +    }

s/maybeScratch/maybeReg/ or even anyReg

@@ +475,5 @@
> +    operator TypedOrValueRegister() const { return output_; }
> +};
> +
> +// Like AutoScratchRegister, but if the |output| has a scratch register,
> +// use that.

// Like AutoScratchRegister, but reuse a register of |output| if possible.
Attachment #8819554 - Flags: review?(hv1989) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9af1b0ed3d
part 1 - Split up BaselineCacheIR.{cpp,h}. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c52c4f524ed
part 2 - Add a mechanism to share codegen for cache ops between Baseline/Ion. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d6b9e5dc4e7
part 3 - Optimize type guards if we know they always succeed. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/12611ebec480
part 4 - Add OperandLocation::Constant since Ion ICs can have constant operands. r=nbp
Keywords: leave-open
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb264cab3a1
part 5 - Factor out restoreInputState from emitFailurePath. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3b8d218cc7
part 6 - Add helpers for pushing/popping values. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/dec599fbdaed
part 7 - Make restoreInputState work for Ion ICs. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/308412053019
part 8 - Rename BaselineCacheIR.{cpp,h} to BaselineCacheIRCompiler.{cpp,h}. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/c845f579be3a
part 9 - Minor cleanup. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/732bfa4cc97a
part 10 - Add initInputLocation overloads for register types used by Ion. r=evilpie
(In reply to Hannes Verschore [:h4writer] from comment #38)
> Should we keep these "empty/unused" spaces in a list that during spilling
> will get reused again?
> Instead of defaulting to pushing to the end of the stack?
> 
> => I assume this would be followup bug if deemed important enough.

Good point, I will file a follow-up bug. Note that we limit the number of IR ops and operands, so it shouldn't get *too* bad, and the stack is empty when we make calls (so it shouldn't affect our recursion limits), but I agree it might be nice to fix this.

> Feel free to ignore. But can we rename orig -> dest? I made it easier to
> read the code beneath. I know that orig is also correct. But reading
> "orig.kind" makes it easy to think that is the source ;).

Ah right, I can see how |orig| is confusing. I renamed it to dest.

> can you add debug asserts that cur.valueReg()->typeReg() ==
> orig.payloadType()

Unfortunately not because valueReg is a ValueOperand and we don't track any type information for Value OperandLocations.

> Can you assert here that "cur.payloadType() == orig.payloadReg()"

Yes! Done.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a7dbaf2e06f
part 11 - GetPropIRGenerator changes to support idempotent Ion ICs. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0a60b26e25d
part 12 - Move codegen for some ops to shared code. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/db73f66731af
part 13 - Share codegen of ops that use the output register. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca40a39ca612
part 14 - Add MacroAssembler::storeRegsInMask. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/bce06174a133
part 15 - Rename JitFrame_IonAccessorIC to JitFrame_IonICCall. r=nbp
(In reply to Hannes Verschore [:h4writer] from comment #41)
> First a suggestion: can we rename emitXXXResult to emitXXXToOutput() 

I'm not sure about *ToOutput (for instance CallScriptedGetterToOutput), maybe just *Output? We could also drop the suffix and have shorter names, but I kind of like seeing it when reading code. Wondering if others agree.

> I think it would be better if the
> constructor and destructor call the alloc.allocateFixedRegister and
> alloc.releaseRegister directly and remove these two Maybe<AutoXXXX>.

Excellent suggestion, it's nicer/simpler this way.

(Addressed all other comments.)
(In reply to Jan de Mooij [:jandem] from comment #47)
> (In reply to Hannes Verschore [:h4writer] from comment #41)
> > First a suggestion: can we rename emitXXXResult to emitXXXToOutput() 
> 
> I'm not sure about *ToOutput (for instance CallScriptedGetterToOutput),
> maybe just *Output? We could also drop the suffix and have shorter names,
> but I kind of like seeing it when reading code. Wondering if others agree.

IMHO emitXXXResult or emitXXXOutput is the same. As a result if we are debating that, I would just leave it.
It is the "To" I find important. Which would describe what it does. Else you need to look inside the code to know that it writes to output. Now if I'm the only person that think that way, we can just leave it. Familiarity eventually makes the reader aware for the specific co-notation "Result" has.

> > I think it would be better if the
> > constructor and destructor call the alloc.allocateFixedRegister and
> > alloc.releaseRegister directly and remove these two Maybe<AutoXXXX>.
> 
> Excellent suggestion, it's nicer/simpler this way.
> 
> (Addressed all other comments.)

Thanks!
Comment on attachment 8819997 [details] [diff] [review]
Part 20 - Remove old GetProperty IC

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

Approve!
Attachment #8819997 - Flags: review?(hv1989) → review+
Comment on attachment 8819994 [details] [diff] [review]
Part 19 - Flip the flag

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

Wohooo!
https://www.youtube.com/watch?v=bn2Z_qay03I
Attachment #8819994 - Flags: review?(hv1989) → review+
Comment on attachment 8819960 [details] [diff] [review]
Part 16 - Add IC infrastructure

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

LGTM
Attachment #8819960 - Flags: review?(hv1989) → review+
Comment on attachment 8819992 [details] [diff] [review]
Part 18 - Add optimized stubs

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

Thanks for answers on IRC.

Can you update http://searchfox.org/mozilla-central/source/js/src/jit/CacheIR.h#21 and include information about the Ion ics and e.g. that differently to the baseline ICS we bake the pointers?

::: js/src/jit/CacheIR.cpp
@@ +1077,5 @@
> +    if (obj->is<TypedArrayObject>() &&
> +        idVal_.toNumber() >= double(obj->as<TypedArrayObject>().length()))
> +    {
> +        return false;
> +    }

Is this what you and evilpie were discussing yesterday?

@@ +1103,1 @@
>      return true;

Good find!

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +111,5 @@
> +#undef DEFINE_OP
> +};
> +
> +class MOZ_RAII AutoSaveLiveRegisters
> +{

This can also use a description on what it does and when it needs to get used.

@@ +923,5 @@
> +
> +    // Try to reuse a previously-allocated CacheIRStubInfo.
> +    CacheIRStubKey::Lookup lookup(kind, ICStubEngine::IonIC,
> +                                  writer.codeStart(), writer.codeLength());
> +    CacheIRStubInfo* stubInfo = jitZone->getIonCacheIRStubInfo(lookup);

Since we bake in the stubFields, we should also check them when trying to match? Or do we test that somewhere else?

@@ +938,5 @@
> +        if (!stubInfo)
> +            return false;
> +
> +        CacheIRStubKey key(stubInfo);
> +        if (!jitZone->putIonCacheIRStubInfo(lookup, key))

Note if this fails, does it mean we have a memory leak, since we don't free stubInfo?? (Same issue in AttachBaselineCacheIRStub)

::: js/src/jit/JitCompartment.h
@@ +391,1 @@
>  };

These items needs some comments on what they represent.
Attachment #8819992 - Flags: review?(hv1989) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5220313f87ff
part 16 - Add IonIC infrastructure. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c3b8297096f
part 17 - Barrier reads from typed objects so we can attach a typed object IC stub. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/efb8b9a6a1a4
part 18 - Add IonCacheIRCompiler to compile Ion IC stubs from CacheIR. r=h4writer
(In reply to Hannes Verschore [:h4writer] from comment #52)
> Is this what you and evilpie were discussing yesterday?

I don't think so, but maybe I forgot.

> Since we bake in the stubFields, we should also check them when trying to
> match? Or do we test that somewhere else?

The CacheIRStubInfo stores the IR + information about the stub fields for the GC. For Ion we just keep a set of CacheIRStubInfos we allocated so we don't allocate the same CacheIRStubInfo instances over and over (this saves memory). We don't have to compare the stub fields because they're not stored in the CacheIRStubInfo.

> @@ +938,5 @@
> > +        CacheIRStubKey key(stubInfo);
> > +        if (!jitZone->putIonCacheIRStubInfo(lookup, key))
> 
> Note if this fails, does it mean we have a memory leak, since we don't free
> stubInfo?? (Same issue in AttachBaselineCacheIRStub)

CacheIRStubKey has a UniquePtr<CacheIRStubInfo>. putIonCacheIRStubInfo will "steal" this pointer if it succeeds (see the Move(key)) and if it fails the UniquePtr in CacheIRStubKey will free it when the CacheIRStubKey is destructed.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4cf2f3d267
part 19 - Enable the new IonGetPropertyIC that's based on CacheIR. r=h4writer
I think AWFY so far confirms these patches have little effect on Octane/Sunspider/Kraken.

There's a win on the "misc-apply-array-headroom" assorted test. I have some ideas what might have caused that, but I'll double check.

Some TypedObject tests regressed, that's probably bug 1325361. There's a small jpeg2000 regression, I think that's bug 1323096 comment 1. I'll fix these next.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/981fd29c5701
followup - Make idempotent ICs handle unshadowed DOM proxies to fix Dromaeo regressions. r=evilpie on IRC
the code which landed in comment 54 caused a lot of leaks:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=46f08777b1f39ec6cc734d379807d0196745a31c

the problem is these don't show up in normal automation due to an error in log parsing which I planned to land this morning.
These leaks are weird: that push adds code but most of it is disabled until the next push, and we leaked WasmModules but these patches are unrelated to wasm. I'll dig more.
The leaks were unrelated, adding a file just revealed it due to unified builds. Bug 1313351 fixes the leak.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/012c06f335f5
part 16 - Add IonIC infrastructure. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d769d7b8933
part 17 - Barrier reads from typed objects so we can attach a typed object IC stub. r=bhackett
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4be622a00d6
part 18 - Add IonCacheIRCompiler to compile Ion IC stubs from CacheIR. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/547a8c1acff1
part 19 - Enable the new IonGetPropertyIC that's based on CacheIR. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce7bd0b486c4
followup - Make idempotent ICs handle unshadowed DOM proxies to fix Dromaeo regressions. r=evilpie on IRC
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b341e3b719a
followup - Fix the --disable-ion build. r=me
Could Part19 have caused this regression?

== Change summary for alert #4619 (as of December 23 2016 22:41 UTC) ==
Regressions:

 52%  misc 0.8 typedobj-simple-struct-typedobj linux32 opt shell     51.02 -> 77.56

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4619
Flags: needinfo?(jdemooij)
(In reply to Hannes Verschore [:h4writer] from comment #67)
> Could Part19 have caused this regression?

Yep, bug 1325361 has a fix for this, I'll push it today. Keeping the NI for now.
Depends on: 1326150
Depends on: 1326157
(In reply to Jan de Mooij [:jandem] from comment #68)
> Yep, bug 1325361 has a fix for this, I'll push it today. Keeping the NI for
> now.

Yes, we're back to normal.
Flags: needinfo?(jdemooij)
When we have an idempotent IC and we fail to attach a stub, we invalidate but we should also return immediately before we do the GetProperty call.

The old code had this return-statement but it got lost when I ported this code. This was causing a browser test failure on Try - we were calling a getter twice instead of once. I didn't manage to write a shell test for this.
Attachment #8824666 - Flags: review?(hv1989)
Attachment #8824666 - Flags: review?(hv1989) → review+
Depends on: 1330248
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/daca1a6156ab
followup - Add a missing return that got lost in the rewrite. r=h4writer
I'll file a new bug for part 20, the code removal. Probably next week after the merge.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1334356
You need to log in before you can comment on or make changes to this bug.