Ted Campbell [:tcampbell] Reporter
	Description • 7 years ago

Now that CacheIR supports super property reads, we should add Ion support for this as well. A use-case for this is overriding methods and then calling the base implementation.

> class Base {
>   foo() {
>     // Do stuff...
>   }
> }
> 
> class Derived extends Base {
>   foo() {
>     // Enhancements...
>     super.foo();
>   }
> }

Super property sets are unusual so don't bother until there are better motivating examples.

Opcodes to support:
 - JSOP_GETPROP_SUPER
 - JSOP_GETELEM_SUPER
 - JSOP_SUPERBASE

	Tom S [:evilpie] Assignee
	Comment 2 • 7 years ago

This doesn't actually work, but I am not sure why. Implementing JSOP_SUPERBASE is not obvious to me, so I would suggest we only support the function() && function()->allowSuperProperty() case.

Anyway, for some reason HomeObject in Ion currently returns global lexical environment?! (I removed the GetPrototypeOf to make this easier to debug)

I run the testcase with --ion-inlining=off to make sure we don't end up with inlining.

	Tom S [:evilpie] Assignee
	Comment 3 • 7 years ago

Found the issue, LIRGenerator::visitHomeObject was creating LFunctionEnvironment! Anyway, JSOP_SUPERBASE is far from correct still and I should probably implement JSPROP_GETELEM_SUPER.

	Tom S [:evilpie] Assignee
	Comment 4 • 7 years ago

Working patch for GETPROP_SUPER and GETELEM_SUPER. Still need to implement HomeObjectSuperBase instead of GetPrototypeOf: I want to just bailout for everything that isn't a static object prototype. Do we need to push a typebarrier or something like?

	Tom S [:evilpie] Assignee
	Comment 5 • 7 years ago

I think this patch is mostly correct now. I already added a test for HomeObjectSuperBase, but we probably need more. try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=cba28d7789a29bbe90a2548543f37433737e000c, but I think the test coverage is just not very good.

	Tom S [:evilpie] Assignee
	Comment 6 • 7 years ago

I was also wondering if we need to do anything about typebarriers in the IonBuilder?

	Tom S [:evilpie] Assignee
	Comment 7 • 7 years ago

Comment on attachment 8916247 [details] [diff] [review]
WIP v2

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

Please give this a quick look especially IonCacheIRCompiler.cpp and IonIC.cpp and the previous question about barriers. Thanks!

	Jan de Mooij [:jandem]
	Comment 8 • 7 years ago

Comment on attachment 8916247 [details] [diff] [review]
WIP v2

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

::: js/src/jit/CodeGenerator.cpp
@@ +3560,5 @@
> +    Register homeObject = ToRegister(lir->homeObject());
> +    Register output = ToRegister(lir->output());
> +
> +    masm.loadObjProto(homeObject, output);
> +    bailoutCmpPtr(Assembler::BelowOrEqual, output, ImmWord(1), lir->snapshot());

What do you think about using an OOL path? Even though it's an edge case I'm a bit worried about repeated bailouts.

::: js/src/jit/IonBuilder.cpp
@@ +9801,5 @@
> +}
> +
> +
> +AbortReasonOr<Ok>
> +IonBuilder::jsop_getprop_super(PropertyName* name)

We could probably use more of our getprop optimizations here, but we can do that later. Same for using an untyped result type for now.

@@ +9809,5 @@
> +
> +    MConstant* id = constant(StringValue(name));
> +    auto* ins = MGetPropSuperCache::New(alloc(), obj, receiver, id);
> +    current->add(ins);
> +    current->push(ins);

We should use pushTypeBarrier here and below, just to improve our type information when the result is used later. Without a barrier it's always "Any Value".

::: js/src/jit/IonIC.cpp
@@ +201,5 @@
> +
> +    bool attached = false;
> +    if (ic->state().canAttachStub()) {
> +        RootedValue val(cx, ObjectValue(*obj));
> +        // IonBuilder calls PropertyReadNeedsTypeBarrier to determine if it

We can remove this comment/code I think when this IC always has a type barrier.

	Jan de Mooij [:jandem]
	Comment 9 • 7 years ago

Thanks for working on this btw! Great to have this fixed.

	Tom S [:evilpie] Assignee
	Comment 10 • 7 years ago

Thanks for the review. Implementing the OOL HomeObjectSuperBase is easier than implementing the bailout \o/ I extended the test a bit and now I get some assertion failure with --ion-eager: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c59070e6a23bec0f905d863c3ed7e2588fab81e&selectedJob=135949501. Looks like we use 0 as this together with OSR maybe? I am not sure how this could happen, unless we use the wrong operand somewhere.

>js --ion-eager --ion-offthread-compile=off --no-threads jit-test/tests/ion/super-prop.js
Error: Assertion failed: got (new Number(0)), expected ({})

	Ted Campbell [:tcampbell] Reporter
	Comment 11 • 7 years ago

Problem with the baseline code at: https://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/js/src/jit/BaselineCompiler.cpp#2324

We pop a temporary value that is used to invoke the BaselineIC, but this doesn't exist in the Ion variant. When Ion bails out for a TYPESET, it rejoins the baseline just after the BaselineIC would run. Unfortunately, with the JSOP_GETELEM_SUPER written as is in Baseline, we execute the baseline-specific frame.pop() on the Ion layout and cause stack misalignment.

Note that this defect also applies to the SETPROP_SUPER and SETEELEM_SUPER baseline implementation.

	Tom S [:evilpie] Assignee
	Comment 12 • 7 years ago

This patch really hasn't changed since WIP v3, but the next patch contains the important Baseline changes that keeps the stack aligned after bailing out from Ion.

	Tom S [:evilpie] Assignee
	Comment 13 • 7 years ago

The stack handling previously was just fine in Baseline, because we would never Ion compile and thus bailout from ion to baseline. However now the extra value that is kept alive across the stub needs special handling, we pad the ion->baseline bailout frame with a fake value that we can pop.

	Tom S [:evilpie] Assignee
	Comment 14 • 7 years ago

Oh I adjusted the test a little bit: I removed the nondeterminism of using Math.random.

	Tom S [:evilpie] Assignee
	Comment 15 • 7 years ago

Attached the wrong version.

	Jan de Mooij [:jandem]
	Comment 16 • 7 years ago

Comment on attachment 8917852 [details] [diff] [review]
v1 - Implement super.property in Ion

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

Great. Sorry for the delay, Tom.

::: js/src/jit/CodeGenerator.cpp
@@ +12939,5 @@
>      // Call into the VM for lazy prototypes.
>      masm.branchPtr(Assembler::Equal, scratch, ImmWord(1), ool->entry());
>  
>      masm.moveValue(NullValue(), out);
> +    masm.printf("null prototype\n");

rm printf

	Jan de Mooij [:jandem]
	Comment 17 • 7 years ago

Comment on attachment 8917893 [details] [diff] [review]
v1 - Fix GetElemSuper stack handling in baseline

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

r=me with comments addressed.

::: js/src/jit/BaselineCompiler.cpp
@@ +2320,5 @@
>      ICGetElem_Fallback::Compiler stubCompiler(cx, /* hasReceiver = */ true);
>      if (!emitOpIC(stubCompiler.getStub(&stubSpace_)))
>          return false;
>  
> +    frame.pop(); // This value is also popped in the bailout code.

I'd add "in InitFromBailout." or something to be more specific.

::: js/src/jit/BaselineIC.cpp
@@ +919,5 @@
>      EmitRestoreTailCallReg(masm);
>  
>      // Super property getters use a |this| that differs from base object
>      if (hasReceiver_) {
> +        // State: R0: index R1: receiver stack: obj

This comment might be a bit ambiguous. Maybe:

// State: index in R0, receiver in R1, obj on the stack

@@ +928,2 @@
>          masm.pushValue(R1);
> +        masm.pushValue(Address(masm.getStackPointer(), sizeof(Value) * 2));

I think here for the expression decompiler to work we should first pop |obj| so the stack actually looks like index/receiver/obj instead of obj/index/receiver/obj. Maybe you can:

(1) pushValue(R1)
(2) pushValue the object currently on the stack (at offset sizeof(Value))
(3) storeValue(R0, ...) to overwrite |obj| with |index|.

You get the idea.

	Tom S [:evilpie] Assignee
	Comment 18 • 7 years ago

I would like to test the decompiler code, but I am not actually sure how in this case. I added some obvious tests for this that already pass.

	Ted Campbell [:tcampbell] Reporter
	Comment 19 • 7 years ago

Add the following test to basic/expression-autopsy.js

> check_one("super[1]",
>           function() {
>             class X extends Object {
>               foo() {
>                 return super[1]();
>               }
>             }
>             new X().foo();
>           },
>           " is not a function");

I should have checked this in originally when I hit this =\

	Pulsebot
	Comment 20 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e8b051b15f8
Fix GetElemSuper stack handling in baseline. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/82bdb5c8e75d
Implement super.property in Ion. r=jandem

	Tom S [:evilpie] Assignee
	Comment 21 • 7 years ago

I am not convinced that the expression decompiler stack handling is detectable. So I pushed this to finally get it landed, but we can surely iterate on this. I also need to remember to open a follow-up bug on more optimization opportunities.

	Stefan Hindli [:stefan_hindli]
	Comment 22 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1e8b051b15f8
https://hg.mozilla.org/mozilla-central/rev/82bdb5c8e75d