1317703 - Port Baseline native getter stub to CacheIR

Status: RESOLVED FIXED

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

Description

[Jan de Mooij [:jandem]]

Bug 1310125 did this for scripted getters (assuming it sticks), this bug is for native getters. Hopefully this will be much simpler as most of the infrastructure for call stubs etc is now in place.

Baseline's TryAttachNativeGetAccessorPropStub is very complicated and hard to understand atm, because it has to deal with getters and window proxies and DOM proxies. I hope we can disentangle this and structure this code more like the Ion IC, with separate paths for (DOM) proxies.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

* This doesn't convert the GetProp_CallNativeGlobal stub that's used for JSOP_GETNAME, once we convert that one we can remove more code.

* The old CallNative stub has some code to stash the object on the stack for the decompiler. This was added in bug 1153458 to avoid a decompiler assertion failure, but that assert has been turned into an if-statement so pushing for the decompiler is no longer necessary.

* I moved the "innerization" optimization to GetPropIRGenerator::tryAttachWindowProxy, to make it easier to understand than in the current code.

* I fixed a related issue in IonBuilder: if we innerize in IonBuilder, we only want to look for Baseline stubs that performed the same optimization. If we don't innerize in IonBuilder, we don't want to consider stubs that performed this optimization. This complicates the pattern matching a bit, but I confirmed it still works and it fixes a pre-existing issue.

Comment 2

[Jan de Mooij [:jandem]]

bz, would you mind reviewing this patch too, especially the innerization/WindowProxy changes (see comment 1)? I don't think anyone else knows about this stuff.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8811726 [details] [diff] [review]
Patch

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

::: js/src/jit/SharedIC.cpp
@@ +2515,5 @@
>          ICGetPropCallDOMProxyNativeCompiler compiler(cx, kind, info->engine(), monitorStub, proxy, holder,
>                                                       callee, info->pcOffset());
>          newStub = compiler.getStub(compiler.getStubSpace(info->outerScript(cx)));
>      } else {
> +        return true;

FWIW, this code (and some other parts) can be simplified a bit now, but the rest of this code will (hopefully) also be removed soon, so I decided to leave it like this.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8811726 [details] [diff] [review]
Patch

The "innerized" arg to BaselineInspector::commonGetPropFunction could use some documentation in the header.

It might be good to document that tryAttachWindowProxy only really attaches (some kinds of?) JSNative getters where the receiver is the WindowProxy.

I think this all works, but I haven't really sorted out the details of the CacheIR stuff yet...

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 8811726 [details] [diff] [review]
Patch

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

Nice. So little code to allow native getters! I almost thought you only implemented the windows proxy stuff only.
Good stuff!

::: js/src/jit/BaselineCacheIR.cpp
@@ +995,5 @@
>        case GuardClassKind::UnmappedArguments:
>          clasp = &UnmappedArgumentsObject::class_;
>          break;
> +      case GuardClassKind::WindowProxy:
> +        clasp = cx_->maybeWindowProxyClass();

Do we need to assert that clasp is non-empty?

@@ +1157,5 @@
> +    // We use ICTailCallReg when entering the stub frame, so ensure it's not
> +    // used for something else.
> +    Maybe<AutoScratchRegister> tail;
> +    if (allocator.isAllocatable(ICTailCallReg))
> +        tail.emplace(allocator, masm, ICTailCallReg);

I think we should create "AutoEnterStubFrame" that does this.
I noticed this also in evilpie his patch and I think it would be more elegant with such a RAII.

::: js/src/jit/BaselineInspector.cpp
@@ +832,5 @@
>          &stub->stubInfo()->getStubField<JSObject*>(stub, offset)->as<JSFunction>();
>  
> +    Shape* thisGlobalShape = nullptr;
> +    if (getter->isNative() &&
> +        receiver.shape &&

Style nit: Any reason this is not on the previous line. That won't make 80chars

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> Do we need to assert that clasp is non-empty?

There's a MOZ_ASSERT(clasp) after the switch.

> I think we should create "AutoEnterStubFrame" that does this.

Agreed. I filed bug 1320118.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/629069be312e
Port Baseline native getter stub to CacheIR. r=h4writer,bz

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/629069be312e