Closed Bug 1326067 Opened 3 years ago Closed 3 years ago

Port Baseline SetProp IC to CacheIR

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

Now that the GetProp/GetElem conversion is done, we can do the same for SetProp and then SetElem. Fortunately, these ICs don't have as many stubs as GetProp and we should be able to reuse a lot of the GetProp/GetElem code.
The Baseline SETPROP IC currently returns the RHS in R0. We don't do this for SETELEM and the Ion IC also doesn't do this. This patch just changes SETPROP to be like SETELEM (leave the RHS on the stack).

This will make it much easier to unify the stubs with Ion. I also think we could now simplify the Baseline SETPROP stubs a bit more, because we no longer need to save R1, but I didn't do that here as these stubs will be rewritten anyway.
Attachment #8822450 - Flags: review?(hv1989)
This patch adds SetPropIRGenerator and uses it for plain data properties.

The main difficulty was adding support for type update ICs, I implemented this as BaselineCacheIRCompiler::callTypeUpdateIC. After we convert the other ICs, we can remove some code like EmitCallTypeUpdateIC that's currently duplicated for each architecture. callTypeUpdateIC is simpler and more efficient than the old code I think - we used to have these EmitStowICValues/EmitUnstowICValues calls that we no longer need now.

The old code handled native objects and unboxed expando objects with the same code. I split these up in SetPropIRGenerator, it makes the code a lot more readable. Also, similar to GetProp/GetName, just having these separate tryAttach* methods makes the code a lot nicer than before.

When we convert the Ion IC, we will probably have to make some small changes to the IR, because the Ion IC handles type checking differently. We can cross that bridge when we get there though. There are only a handful of things that need type updating so it shouldn't be too bad.
Attachment #8822642 - Flags: review?(hv1989)
Rebased and fixes a minor bug.
Attachment #8822642 - Attachment is obsolete: true
Attachment #8822642 - Flags: review?(hv1989)
Attachment #8823208 - Flags: review?(hv1989)
Attachment #8822450 - Flags: review?(hv1989) → review+
Comment on attachment 8823208 [details] [diff] [review]
Part 2 - Port SetProp_Native stub

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

Basic and overall design looks good. Keep up the good work! To unblock I'm already giving r+.
I'll re-read again on Thursday for myself. Such a patch is hard to grasp in one sitting.

::: js/src/jit/CacheIR.cpp
@@ +1515,5 @@
> +    EnsureTrackPropertyTypes(cx_, obj, id);
> +    if (!PropertyHasBeenMarkedNonConstant(obj, id)) {
> +        *isTemporarilyUnoptimizable_ = true;
> +        return false;
> +    }

Good catch!

::: js/src/jit/CacheIRCompiler.cpp
@@ +82,5 @@
> +        break;
> +      case OperandLocation::PayloadReg:
> +        masm.tagValue(loc.payloadType(), loc.payloadReg(), reg);
> +        MOZ_ASSERT(!currentOpRegs_.has(loc.payloadReg()), "Register shouldn't be in use");
> +        availableRegs_.add(loc.payloadReg());

:D
Attachment #8823208 - Flags: review?(hv1989) → review+
I pulled the type guard for the RHS value out of StoreUnboxedProperty, so StoreUnboxedProperty is now infallible. This is a nice simplification and later we can use EmitGuardUnboxedPropertyType for unboxed arrays as well.
Attachment #8823651 - Flags: review?(evilpies)
Comment on attachment 8823651 [details] [diff] [review]
Part 3 - Port SetProp_Unboxed stub

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

Looks good to me.
Attachment #8823651 - Flags: review?(evilpies) → review+
Priority: -- → P1
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/221a84322295
part 1 - Change Baseline SetProp IC to leave the RHS value on the stack instead of returning it. r=h4writer
Keywords: leave-open
I split this into 2 separate ops: StoreTypedObjectReferenceProperty and StoreTypedObjectScalarProperty. These are quite different so this simplifies things.

I also changed the TypedObject code in DoTypeUpdateFallback. We now do a bit more work there, but we no longer have to store flags on the IC stub for this and we only need to do this for null/undefined.

Finally, I added a template argument to StoreToTypedArray (and renamed it to BaselineStoreToTypedArray), because we always called that with an Address for the RHS value, but here we want to call it with a ValueOperand. I think when we convert the TypedArray SetElement stub, we can change this to always take a ValueOperand.
Attachment #8824088 - Flags: review?(evilpies)
Blocks: 1091978
Comment on attachment 8824088 [details] [diff] [review]
Part 4 - Port SetProp_TypedObject

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

That's a lot of complicated code for something that is not even used in the real world. I would almost suggest we don't land this.

::: js/src/jit/BaselineIC.cpp
@@ +294,5 @@
> +
> +            TypeDescr* fieldDescr = &structDescr->fieldDescr(fieldIndex);
> +            ReferenceTypeDescr::Type type = fieldDescr->as<ReferenceTypeDescr>().type();
> +            if (type == ReferenceTypeDescr::TYPE_ANY) {
> +                // Ignore undefined values, which are included implicitly in type

Took me a while to understand this compared to the previous code, but I think this way is better.
Attachment #8824088 - Flags: review?(evilpies) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d72e00c604a
part 2 - Convert Baseline setslot IC stub to CacheIR. r=h4writer
That's a pre-existing thing, I filed bug 1331452 yesterday.

The CGC builds are very sensitive to random code changes, sadly.
Depends on: 1331452
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/18ea64fbf389
part 2 - Convert Baseline setslot IC stub to CacheIR. r=h4writer
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f94d334ae5a9
followup - Add basic/destructuring-iterator.js jit-test to cgc-jittest-timeouts.txt. r=me
Attached patch Part 5 - SettersSplinter Review
This is very similar to the code we have for getters.

We use a CacheIR_Updated stub for this, even though we don't strictly need the type updating for setters. It's similar to how we always allocate a CacheIR_Monitored stub for GetProp/GetElem/GetName stubs, even those that don't do type monitoring.
Attachment #8828405 - Flags: review?(evilpies)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a16c508abfd0
part 4 - Port Baseline TypedObject SetProp stub to CacheIR. r=evilpie
Comment on attachment 8828405 [details] [diff] [review]
Part 5 - Setters

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

Very nice.
Attachment #8828405 - Flags: review?(evilpies) → review+
I got a crash with Nightly on Android in TypeUpdateFallback. The problem is that EmitStubGuardFailure on ARM/MIPS assumes R2 is available as scratch, but that's not true when we call a type update stub from CacheIR stubs.

The fix is simple: just use masm.jump(address) so we don't need a scratch register.

r? from both Hannes and evilpie to improve the odds of getting this landed before the merge :)
Attachment #8829096 - Flags: review?(hv1989)
Attachment #8829096 - Flags: review?(evilpies)
Comment on attachment 8829096 [details] [diff] [review]
Part 6 - Fix EmitStubGuardFailure

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

All of these functions basically look identical now, can't we share that code?
Attachment #8829096 - Flags: review?(evilpies) → review+
(In reply to Tom Schuster [:evilpie] from comment #24)
> All of these functions basically look identical now, can't we share that
> code?

Good point. My plan was to wait until the conversion to CacheIR is done and then remove/simplify all of this. There will be a lot of code in SharedICHelpers-*.h that we no longer need, like EmitStowICValues, EmitUnstowICValues, EmitCallTypeUpdateIC etc, and a lot of other functions can probably be shared or inlined (because there will be a single CacheIR call site).
I noticed another difference with the old code: when we set an unboxed expando property, we used to pass the unboxed object to the type update IC and now we pass the UnboxedExpandoObject.

Passing the unboxed object is a bit annoying with the CacheIR stubs. I think a nicer fix is to store the group in the update stub, and then use that in DoTypeUpdateFallback. This way, we don't need to change the Store(Fixed|Dynamic)Slot IR and codegen.

Later on we will also need this information to generate Ion SetProp stubs from the same CacheIR.

Brian, the most interesting part is the DoTypeUpdateFallback change, the rest is just plumbing to pass the group to that code.
Attachment #8829111 - Flags: review?(bhackett1024)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f03c71ed58
part 6 - Fix EmitStubGuardFailure to not clobber registers on ARM/MIPS. r=evilpie
Attachment #8829096 - Flags: review?(hv1989)
Attachment #8829111 - Flags: review?(bhackett1024) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f91b100895ab
part 7 - Fix type update code to use the unboxed object instead of the UnboxedExpandoObject. r=bhackett
I still have to land part 5, but I'll file a new bug for the remaining work to simplify release tracking.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1332933
You need to log in before you can comment on or make changes to this bug.