Closed Bug 1341067 Opened 7 years ago Closed 7 years ago

Port Ion SetProp/SetElem IC to CacheIR

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
Once bug 1338828 is fixed, the Baseline CacheIR IC will support all cases we handle in Ion, so we can replace the Ion code.

The attached patch makes this change. Most of the work here was implementing the various store instructions. (Because Baseline uses type update ICs and Ion uses a different mechanism, it's hard to share these ops.) The code for typed arrays and dense elements is based on the current Ion code.

Things mostly worked after that. There was one issue with aliased input registers: multiple input operands can have the same register in Ion in cases like this:

  obj.x = obj;
  arr[i] = i;

To avoid complicating the primitive IC regalloc, the IC register allocator checks whether inputs alias each other and in that case it spills one of them to ensure every register is in use by at most one operand. I knew this situation was going to come up (our manual IC regalloc also had to deal with this and it was error prone) and I'm glad this works well.

I also had to teach the allocator about double registers for things like |arr[i] = double|. We don't allocate/touch double registers so we only have to add a new variant to the tagged union.

This patch deduplicates a lot of IC code:

 22 files changed, 1118 insertions(+), 2204 deletions(-)

Performance on Octane/Kraken/Sunspider/assorted is mostly the same as before, maybe a small win on some tests but it's hard to say.

After this the only IonCaches ICs left are GetName and BindName and these should be much easier to port.
Attachment #8839193 - Flags: review?(hv1989)
Blocks: 1341071
Note that this patch doesn't add post barriers like we have in Baseline. This isn't necessary but we should fix that, I filed bug 1341071.
Comment on attachment 8839193 [details] [diff] [review]
Patch

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

Like discussed online it would be nice if we could share more between the different IRCompilers. Now I'm less agitated by it due to some reasons.
The reason I wanted to make sure they look alike, is to make sure we only have to implement it once and have it working everywhere. But I forgot that I forgot one rule here. It needs to be simple to share them on first creation. But it should be possible to optimize a stub for a specific engine. And that is what is happening here. We have a more optimized version for Ion. And that is causing the differences.
=> If we can remove the difference between some stubs without removing the optimizations I'm all ear ;).

I can only imagine how much effort you had to put into this given how much time it took me to review. Nice work! Thanks!

::: js/src/jit/IonCacheIRCompiler.cpp
@@ +124,5 @@
>  
>      void prepareVMCall(MacroAssembler& masm);
>      MOZ_MUST_USE bool callVM(MacroAssembler& masm, const VMFunction& fun);
>  
> +    MOZ_MUST_USE bool emitAddAndStoreSlotShared(CacheOp op);

protected/private I assume.

@@ +1024,5 @@
> +}
> +
> +bool
> +IonCacheIRCompiler::emitAddAndStoreSlotShared(CacheOp op)
> +{

You probably reasoned about this. But what is the reason to have this as one (cacheIR) bytecode VS splitting it into "addSlotShared" and "storeSlotShared". Since we have the "storeSlotShared" bytecodes we would only have to add the "addSlotShared" part? Just curious.

@@ +1204,5 @@
> +    // Compute the address being written to.
> +    LoadTypedThingData(masm, layout, obj, scratch1);
> +    Address dest(scratch1, offset);
> +
> +    BaselineStoreToTypedArray(cx_, masm, type, val, dest, scratch2, failure->label());

This is a baseline function. I.e. I assume we should rename this and move this to something baseline/ion accessible.

::: js/src/jit/IonIC.h
@@ +239,5 @@
> +    Register object() const { return object_; }
> +    ConstantOrRegister id() const { return id_; }
> +    ConstantOrRegister rhs() const { return rhs_; }
> +
> +    Register temp1() const { return temp1_; }

We should probably just call this "temp"?

::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +4840,5 @@
>  
>  void
> +MacroAssembler::storeRegsInMask(LiveRegisterSet set, Address dest)
> +{
> +    ScratchRegisterScope scratch(*this);

Can LiveRegisterSet contain the ScratchReg? Else we should keep the assert ;).
Attachment #8839193 - Flags: review?(hv1989) → review+
Blocks: 1328140
(In reply to Hannes Verschore [:h4writer] from comment #2)
> > +    MOZ_MUST_USE bool emitAddAndStoreSlotShared(CacheOp op);
> 
> protected/private I assume.

Yup, this is in a "private:" section.

> You probably reasoned about this. But what is the reason to have this as one
> (cacheIR) bytecode VS splitting it into "addSlotShared" and
> "storeSlotShared". Since we have the "storeSlotShared" bytecodes we would
> only have to add the "addSlotShared" part? Just curious.

The store part of emitAddAndStoreSlotShared is pretty small and it's different from StoreFixedSlot/StoreDynamicSlot in a number of ways: we don't use a pre-barrier here for instance because it's an initialization instead of a set. It seemed clearer to keep them separate.

> This is a baseline function. I.e. I assume we should rename this and move
> this to something baseline/ion accessible.

Good point, I renamed it to StoreToTypedArray. I think we should move it somewhere else, I'll do that in bug 1340153.

> We should probably just call this "temp"?

Done.

> Can LiveRegisterSet contain the ScratchReg? Else we should keep the assert
> ;).

Actually Try uncovered some problems with these changes so I reverted them. I did remove the scratch-is-not-in-set assert: that's not always true but the code works fine without it.
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4f1db56ebc0
Port Ion SetProp/SetElem IC to CacheIR. r=h4writer
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/03be1ce804a5
followup - Fix non-unified build bustage. r=red
This regressed the sixspeed object-literal-ext-es5 test a lot. That is bug 1339535, the old Ion AddSlot code didn't check newShape->previous() == oldShape, so it didn't run into that. I'll post a patch for that one.
https://hg.mozilla.org/mozilla-central/rev/d4f1db56ebc0
https://hg.mozilla.org/mozilla-central/rev/03be1ce804a5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fabd7eaacb43
followup - Some renaming review nits I forgot to qref. r=me
Depends on: 1343412
I see dromaeo_css wins from this!

== Change summary for alert #5264 (as of February 27 2017 09:00 UTC) ==

Improvements:

  7%  dromaeo_css summary windows7-32 pgo e10s     6301.31 -> 6738.77
  6%  dromaeo_css summary windows7-32 pgo          6166.01 -> 6545
  5%  dromaeo_css summary windows8-64 pgo e10s     6637.28 -> 6999.22
  5%  dromaeo_css summary windows8-64 pgo          6527.82 -> 6869.49

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=5264
Depends on: 1357024
You need to log in before you can comment on or make changes to this bug.