1324566 - Port Baseline GetName IC to CacheIR

Status: RESOLVED FIXED

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

Comment 1

[Tom S [:evilpie]]

Attachment: Port Baseline GetName_Env to CacheIR

I think this is relatively straightforward. I had to change some code around input operands in BaselineCacheIR.cpp, because GetName gets an unboxed object on the stack. This -50 overall lines, but porting the other two ICs should make this a bigger win.

Comment 2

[Tom S [:evilpie]]

Attachment: WIP Port GlobalNameValue

I still need to check this over a bit more, ICGetPropNativeCompiler is really not obvious.

This patch allows us to remove ICGetPropNativeStub completely, but there might be even more stuff we can remove now that I missed.

7 files changed, 82 insertions(+), 407 deletions(-)

Comment 3

[Tom S [:evilpie]]

Mostly I am curious if we need to guard on the group or shape of objects on the prototype chain. For GetProp we usually only do the proto guard if we have an UncacheableProto. Presumably this is not possible here?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #3)
> Mostly I am curious if we need to guard on the group or shape of objects on
> the prototype chain. For GetProp we usually only do the proto guard if we
> have an UncacheableProto. Presumably this is not possible here?

__proto__ mutation may trigger hasUncacheableProto, but these objects are singletons so you may have to put a non-singleton on the proto chain first. However, this is the global right? The global has an immutable proto chain so we can probably just assert that?

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8820420 [details] [diff] [review]
Port Baseline GetName_Env to CacheIR

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

::: js/src/jit/CacheIR.h
@@ +744,4 @@
>      CacheKind cacheKind() const { return cacheKind_; }
>  };
>  
> +// GetPropIRGenerator generates CacheIR for a GerName IC.

GetName

Comment 6

[Jeff Walden [:Waldo]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> However, this is the global right? The global has an immutable proto chain
> so we can probably just assert that?

This is true in the browser embedding, but SpiderMonkey doesn't enforce this requirement on all global objects.  Yet.  I have some patchwork locally to do this, but that patch possibly should wait for a full rethink of global-object creation that would specify the entire prototype chain at the same time (versus the current thing where people must manually JS_SplicePrototype their desired prototype into the chain).

In any event, right now we can't just assert this.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> This is true in the browser embedding, but SpiderMonkey doesn't enforce this
> requirement on all global objects.  Yet.

OK, thanks. Then we should probably check hasUncacheableProto and not attach if that's true - it means someone is messing with the global's prototype chain and if that can't even happen in the browser, I don't think we need to support this.

Comment 8

[Tom S [:evilpie]]

Comment on attachment 8820420 [details] [diff] [review]
Port Baseline GetName_Env to CacheIR

Resetting review for now. This needs to rebased, which removes some ugly code. And the shapes.append call can fail, so I need to avoid that somehow.

Comment 9

[Tom S [:evilpie]]

Attachment: v2 - Port Baseline GetName_Env to CacheIR

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8820899 [details] [diff] [review]
v2 - Port Baseline GetName_Env to CacheIR

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

Looks great! Some suggestions below, sorry for the rebase pain.

::: js/src/jit/BaselineCacheIRCompiler.cpp
@@ +1119,4 @@
>  }
>  
>  bool
> +BaselineCacheIRCompiler::emitLoadEnclosingEnv()

If you rebase this again (sorry this should be the last time..) we can share this op with Ion.

@@ +1197,5 @@
>  }
>  
>  bool
> +BaselineCacheIRCompiler::emitLoadEnvNameFixedSlotResult()
> +{

And use AutoOutputRegister + AutoScratchRegisterMaybeOutput here

@@ +1209,5 @@
> +    masm.load32(stubAddress(reader.stubOffset()), scratch);
> +
> +    // Check for uninitialized lexicals.
> +    BaseIndex slot(obj, scratch, TimesOne);
> +    masm.branchTestMagic(Assembler::Equal, slot, failure->label());

With AutoOutputRegister, I *think* we can now use Load{Fixed,Dynamic}SlotResult and do the branchTestMagic as a separate fallible op, without clobbering |obj| anywhere (the allocator should move it from R0 to another register). What do you think? It would be similar to the CrossCompartmentWrapper thing.

@@ -1221,4 @@
>      // unlimited number of stubs.
>      MOZ_ASSERT(stub->numOptimizedStubs() < MaxOptimizedCacheIRStubs);
>  
> -    MOZ_ASSERT(kind == CacheKind::GetProp || kind == CacheKind::GetElem);

Let's keep this assert for now: when we add SetProp we need to update this code to not use sizeof(ICCacheIR_Monitored).

::: js/src/jit/CacheIR.h
@@ +755,5 @@
> +    jsbytecode* pc_;
> +    HandleObject env_;
> +    HandlePropertyName name_;
> +    ICStubEngine engine_;
> +    CacheKind cacheKind_;

Nit: I think we should add an IRGenerator base class and move most of these members (and writerRef/cacheKind) into it. Later we can then also add (shared) emit* methods to the base class, like emitIdGuard.

I also think we should have ICStubEngine only in GetPropIRGenerator: ideally the IR will be independent of the IC kind. For GetProp we need it for now for scripted getters that we don't support in Ion shared-stubs but if/when we remove shared stubs we can hopefully get rid of it.

@@ +758,5 @@
> +    ICStubEngine engine_;
> +    CacheKind cacheKind_;
> +
> +    GetNameIRGenerator(const GetNameIRGenerator&) = delete;
> +    GetNameIRGenerator& operator=(const GetNameIRGenerator&) = delete;

If we do this in IRGenerator we no longer need it in derived classes AFAIK.

::: js/src/jit/SharedIC.cpp
@@ -373,5 @@
> -      case ICStub::GetName_Env5:
> -        static_cast<ICGetName_Env<5>*>(this)->traceEnvironments(trc);
> -        break;
> -      case ICStub::GetName_Env6:
> -        static_cast<ICGetName_Env<6>*>(this)->traceEnvironments(trc);

Variable number of shapes was a real pain, glad we can get rid of all this boilerplate \o/

Comment 11

[Tom S [:evilpie]]

Attachment: v3 - Port Baseline GetName_Env to CacheIR

I don't think sharing the Load[Fixed]Slot code is going to be very nice. We currently don't have a good way to use the result as input to another instruction. Just adding this special case for two Ops doesn't seem worth it. However seeing as we probably will need this for CrossCompartmentWrapper (bug 1319087) as well, I am going to look into how to do this. At that point we can probably also move this out of BaselineCacheIRCompiler

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8821514 [details] [diff] [review]
v3 - Port Baseline GetName_Env to CacheIR

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

Beautiful.

Comment 13

[Tom S [:evilpie]]

Attachment: Port Baseline GlobalNameValue to CacheIR

Added the hasImmutablePrototype check and optimized the global/holder check to omit the unnecessary load when global == holder.

Comment 14

[Tom S [:evilpie]]

I think we might be running out of registers on x86 ... https://treeherder.mozilla.org/#/jobs?repo=try&revision=f92d952a993b9298f945a6cb5969a774ac81a8cc&selectedJob=33396400

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f75fcf25c4a0
Set available registers after initializing the input locations. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/094c2806ef37
Port Baseline GetName_Env to CacheIR. r=jandem

Comment 16

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a33cd6b281f834217875eac8d7147422847b3d15

Jan provided the first patch over IRC yesterday, I just reviewed it myself. The actual patch had a small change to explicitly give back R0.typeReg() on x86 32bit, because ICStubCompiler::availableGeneralRegs assumes that for one input the R0 payload and type registers are used.

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f75fcf25c4a0
https://hg.mozilla.org/mozilla-central/rev/094c2806ef37

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 8821524 [details] [diff] [review]
Port Baseline GlobalNameValue to CacheIR

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

Great that we don't need any IR/codegen changes for this, and that the "which guards do we emit" logic is now in a single place.

::: js/src/jit/CacheIR.cpp
@@ +1197,5 @@
> +        size_t dynamicSlotOffset = current->dynamicSlotIndex(shape->slot()) * sizeof(Value);
> +        writer.loadDynamicSlotResult(objId, dynamicSlotOffset);
> +    } else {
> +        // Check the prototype chain from the global to the current
> +        // prototype. Ignore the global lexical scope as it doesn' figure

Pre-existing nit: s/doesn'/doesn't/

@@ +1207,5 @@
> +        // Shape guard for global lexical.
> +        writer.guardShape(objId, globalLexical->lastProperty());
> +
> +        // If we are generating a non-lexical GETGNAME stub, we must also
> +        // guard on the shape of the GlobalObject.

Nit: we can remove the first part of this comment, as this is always a non-lexical GETGNAME. The old code had it because it used to be shared with GETPROP stubs.

::: js/src/jit/SharedIC.h
@@ -2464,5 @@
> -        return static_cast<int32_t>(engine_) |
> -              (static_cast<int32_t>(kind) << 1) |
> -              (static_cast<int32_t>(isFixedSlot_) << 17) |
> -              (static_cast<int32_t>(inputDefinitelyObject_) << 18) |
> -              (HeapReceiverGuard::keyBits(obj_) << 19);

Not going to miss this.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4398ee2eea0c
Port Baseline GlobalNameValue to CacheIR. r=jandem

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4398ee2eea0c

Comment 21

[Hannes Verschore [:h4writer]]

Should this now get closed?

Comment 22

[Tom S [:evilpie]]

No, there is still one IC missing.

Comment 23

[Tom S [:evilpie]]

Attachment: WIP Baseline GlobalNameAccessor to CacheIR

Comment 24

[Tom S [:evilpie]]

IsCacheableGetPropCallNative returns false for IsWindow, which is going to be most cases here, not sure if that is a problem? I ran this in the browser and some seems to have that special JIT info that makes it okay to attach.

Comment 25

[Tom S [:evilpie]]

Attachment: Port Baseline GlobalNameAccessor to CacheIR

I think my patch is correct in that regard.
Stats:
11 files changed, 163 insertions(+), 604 deletions(-)
There is probably more stuff related to GetProp that can be removed now, but I didn't check that hard.

Comment 26

[Jan de Mooij [:jandem]]

Comment on attachment 8822041 [details] [diff] [review]
Port Baseline GlobalNameAccessor to CacheIR

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

::: js/src/jit/BaselineIC.cpp
@@ -2510,5 @@
>          attached = true;
>      }
>  
> -    if (!attached && IsGlobalOp(JSOp(*pc)) && !script->hasNonSyntacticScope()) {
> -        if (!TryAttachGlobalNameAccessorStub(cx, script, pc, stub,

We should move the CacheIR IC code here (before the GetEnvironmentName call). Some getters add properties and trigger a shape change, in that case our shape guards always fail if we attach the stub after the fallback code. For GetName that's less relevant, but we have seen it with GetProp.

::: js/src/jit/BaselineInspector.cpp
@@ -904,1 @@
>              if (!AddCacheIRGetPropFunction(stub->toCacheIR_Monitored(), innerized,

We have to fix AddCacheIRGetPropFunction to match the extra shape check we add for the global lexical scope. Can probably skip LoadEnclosingEnvironment + GuardShape if we detect these, just need to make sure we get the right receiver/holder shapes.

Please make sure the getPropTryCommonGetter optimization in IonBuilder::jsop_getgname still works as before for own and inherited properties.

Comment 27

[Tom S [:evilpie]]

Attachment: Inspector support for GlobalNameGetter

There are all kinds of weird conditions in this code that aren't exactly obvious, but I did verify that the optimization works for native getters directly on the global and on the prototype chain.

Comment 28

[Tom S [:evilpie]]

I just noticed I don't guard on the global lexical at all in the Inspector, so something is probably wrong.

Comment 29

[Tom S [:evilpie]]

Attachment: v2 - Inspector support for GlobalNameGetter

I think this version of the patch should match the previous behavior.

Comment 30

[Tom S [:evilpie]]

Attachment: v2 - Port Baseline GlobalNameAccessor to CacheIR

Comment 31

[Tom S [:evilpie]]

Attachment: v2 - Inspector support for GlobalNameGetter

If we can't use script->global() we can just keep baking in holder even when it's the global.

Comment 32

[Jan de Mooij [:jandem]]

Comment on attachment 8822471 [details] [diff] [review]
v2 - Port Baseline GlobalNameAccessor to CacheIR

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

Excellent.

Comment 33

[Jan de Mooij [:jandem]]

Comment on attachment 8822473 [details] [diff] [review]
v2 -  Inspector support for GlobalNameGetter

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

Yeah, this pattern matching is annoying. Hopefully it's just a stopgap until bug 1324561 replaces it.

::: js/src/jit/BaselineInspector.cpp
@@ +731,5 @@
> +    //   GuardShape objId
> +    //   globalId = LoadEnclosingEnvironment objId
> +    //   GuardShape globalId
> +    //   <holderId = LoadObject <holder>>
> +    //   <GuardShape holderId>

Nit: add "CallNativeGetterResult globalId"

@@ +746,5 @@
> +    ObjOperandId globalId = reader.objOperandId();
> +
> +    if (!reader.matchOp(CacheOp::GuardShape, globalId))
> +        return false;
> +    Shape* globalShape = stub->stubInfo()->getStubField<Shape*>(stub, reader.stubOffset());

Nit: maybe add:

MOZ_ASSERT(globalShape->getObjectClass()->flags & JSCLASS_IS_GLOBAL);

@@ +748,5 @@
> +    if (!reader.matchOp(CacheOp::GuardShape, globalId))
> +        return false;
> +    Shape* globalShape = stub->stubInfo()->getStubField<Shape*>(stub, reader.stubOffset());
> +
> +    JSObject* holder = &script->global(); // Todo: Not sure if we want to do this.

I think this is OK. The holder must be the (same) global object.

@@ +758,5 @@
> +        if (!reader.matchOp(CacheOp::GuardShape, holderId))
> +            return false;
> +        holderShape = stub->stubInfo()->getStubField<Shape*>(stub, reader.stubOffset());
> +    }
> +

At this point we can check

  if (holder->as<NativeObject>().lastProperty() != holderShape)
      return true;

|true| to skip this stub and try the next one, as this one will always fail. We used to update Baseline stubs that were outdated, but as we no longer do that we should check this in the inspector.

Comment 34

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8822473 [details] [diff] [review]
v2 -  Inspector support for GlobalNameGetter

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

::: js/src/jit/BaselineInspector.cpp
@@ +776,5 @@
> +        *holderShape_ = holderShape;
> +        *commonGetter = getter;
> +        *globalShape_ = globalShape;
> +
> +        // This is always false, because the getters never live on the globalLexial.

nit: globalLexical

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1832a6e47f1c
Port Baseline GlobalNameAccessor to CacheIR. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85df7c92997
Inspector support for GlobalNameGetter. r=jandem

Comment 36

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1832a6e47f1c
https://hg.mozilla.org/mozilla-central/rev/f85df7c92997