Closed
Bug 1378186
Opened 7 years ago
Closed 7 years ago
Support |super.prop| in Ion
Categories
(Core :: JavaScript Engine: JIT, enhancement, P3)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: tcampbell, Assigned: evilpie)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 6 obsolete files)
457 bytes,
application/x-javascript
|
Details | |
38.54 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
8.66 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: tcampbell → evilpies
Assignee | ||
Comment 1•7 years ago
|
||
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.
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.
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?
Attachment #8916160 -
Attachment is obsolete: true
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.
Attachment #8916183 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
I was also wondering if we need to do anything about typebarriers in the IonBuilder?
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!
Attachment #8916247 -
Flags: feedback?(jdemooij)
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.
Attachment #8916247 -
Flags: feedback?(jdemooij) → feedback+
Comment 9•7 years ago
|
||
Thanks for working on this btw! Great to have this fixed.
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 ({})
Attachment #8916247 -
Attachment is obsolete: true
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.
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.
Attachment #8917027 -
Attachment is obsolete: true
Attachment #8917852 -
Flags: review?(jdemooij)
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.
Attachment #8917856 -
Flags: review?(jdemooij)
Assignee | ||
Comment 14•7 years ago
|
||
Oh I adjusted the test a little bit: I removed the nondeterminism of using Math.random.
Assignee | ||
Comment 15•7 years ago
|
||
Attached the wrong version.
Attachment #8917856 -
Attachment is obsolete: true
Attachment #8917856 -
Flags: review?(jdemooij)
Attachment #8917893 -
Flags: review?(jdemooij)
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
Attachment #8917852 -
Flags: review?(jdemooij) → review+
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.
Attachment #8917893 -
Flags: review?(jdemooij) → review+
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.
Attachment #8917893 -
Attachment is obsolete: true
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 =\
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
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.
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1e8b051b15f8
https://hg.mozilla.org/mozilla-central/rev/82bdb5c8e75d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•