Deoptimization support for accessors.

Deoptimization support for accessors.

Highlights of this CL:

 * Introduced a new opcode in the deoptimizer for a setter stub frame.

 * Added a global setter stub for returning after deoptimizing a setter.

 * We do not need special deopt support for getters, although the getter stub creates an internal frame. The normal machinery works just right for this case, although we generate a stack that can never occur during normal fullcode execution. If this hurts us one day, we can parameterize and reuse the setter deopt machinery.

Committed: https://code.google.com/p/v8/source/detail?r=12328

[Sven Panne, 2012-08-16 13:33:04]

Submitted for an initial review round, ia32 and x64 support is fully functional,
2 (hopefully tiny) things are still missing on ARM and MIPS.

[Michael Starzinger, 2012-08-16 17:20:04]

First round. One real comment in the deoptimizer and a bunch of nits. I just
looked at ia32, but I assume my comments apply to all architectures.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h
File src/builtins.h (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h#new...
src/builtins.h:160: V(SetterStubForDeopt,             STORE_IC, MONOMORPHIC,    
         \
Can we call this "StoreIC_Setter_ForDeopt"?

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
File src/ia32/deoptimizer-ia32.cc (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:700: // The receiver and RHS are expected in
registers by the IC, so they don't
s/IC/StoreIC/

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:702: // of 0 instead of 2.
Drop the "instead of 2" part.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:709: // 1 stack entry for the return address + 4
stack entries from
Can we start this sentence with a word, not a number, to make it a proper
sentence.

Also we should really think about moving these constants in frames-[arch].h at
some point, but that's for a followup CL.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:777: value =
reinterpret_cast<intptr_t>(setter->code());
I don't think this is quite right. What you want here is the code object for the
setter stub, not for the JavaScript setter. So this should be "setter_stub"
instead.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:790: // The RHS was part of the artificial setter
stub environment.
I would use "implicit return value" or "passed value" here instead of RHS,
because RHS only applies to ordinary assignments, it has little meaning for
counting or compound assignments.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:796: Code* setter_stub =
Move this up, you already need if for the code object.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cach...
File src/ia32/stub-cache-ia32.cc (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cach...
src/ia32/stub-cache-ia32.cc:2669: if (setter.is_null()) {
Can we flip the condition, thereby the return point would visually be after the
InvokeFunction part, where it actually needs to be. That's visually more
intuitive (at least for me).

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#new...
src/objects.cc:8307: case Translation::SETTER_STUB_FRAME: {
Can we make that an own case that doesn't print the height at all? This makes it
hard to see when we expect the iterator to give us a height and when not.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#new...
src/objects.cc:8312: opcode == Translation::SETTER_STUB_FRAME ? 2 :
iterator.Next();
Actually, height = 0 for the setter stub, but that doesn't matter if the above
comment is addressed.

[Sven Panne, 2012-08-17 07:27:26]

Addressed feedback comments.

Now the only things missing are 1 tiny stack slot on ARM and a similar one on
MIPS... :-P

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h
File src/builtins.h (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/builtins.h#new...
src/builtins.h:160: V(SetterStubForDeopt,             STORE_IC, MONOMORPHIC,    
         \
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> Can we call this "StoreIC_Setter_ForDeopt"?

Done.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
File src/ia32/deoptimizer-ia32.cc (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:700: // The receiver and RHS are expected in
registers by the IC, so they don't
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> s/IC/StoreIC/

Done here and on other platforms.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:702: // of 0 instead of 2.
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> Drop the "instead of 2" part.

... it's cleaner! ;-) Done here and on other platforms.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:709: // 1 stack entry for the return address + 4
stack entries from
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> Can we start this sentence with a word, not a number, to make it a proper
> sentence.

Done here and on other platforms.

> Also we should really think about moving these constants in frames-[arch].h at
> some point, but that's for a followup CL.

Definitely, but we should then do this for all deoptimizer functions at once.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:777: value =
reinterpret_cast<intptr_t>(setter->code());
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> I don't think this is quite right. What you want here is the code object for
the
> setter stub, not for the JavaScript setter. So this should be "setter_stub"
> instead.

Good catch. Done here and on other platforms.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:790: // The RHS was part of the artificial setter
stub environment.
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> I would use "implicit return value" or "passed value" here instead of RHS,
> because RHS only applies to ordinary assignments, it has little meaning for
> counting or compound assignments.

Done here and on other platforms.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/deoptimiz...
src/ia32/deoptimizer-ia32.cc:796: Code* setter_stub =
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> Move this up, you already need if for the code object.

Done here and on other platforms.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cach...
File src/ia32/stub-cache-ia32.cc (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/ia32/stub-cach...
src/ia32/stub-cache-ia32.cc:2669: if (setter.is_null()) {
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> Can we flip the condition, thereby the return point would visually be after
the
> InvokeFunction part, where it actually needs to be. That's visually more
> intuitive (at least for me).

Good point. Done here and on other platforms.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#new...
src/objects.cc:8307: case Translation::SETTER_STUB_FRAME: {
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> Can we make that an own case that doesn't print the height at all? This makes
it
> hard to see when we expect the iterator to give us a height and when not.

Done.

https://chromiumcodereview.appspot.com/10855098/diff/12048/src/objects.cc#new...
src/objects.cc:8312: opcode == Translation::SETTER_STUB_FRAME ? 2 :
iterator.Next();
On 2012/08/16 17:20:04, Michael Starzinger wrote:
> Actually, height = 0 for the setter stub, but that doesn't matter if the above
> comment is addressed.

Well, it is totally unclear what is *really* meant here: There is a lot of
naming confusion going on, this "height" here is sometimes called
"translation_size", at other places "height" has a different meaning, compare
e.g. LCodeGen::WriteTranslation and Translation::BeginConstructStubFrame. But as
you said, this won't matter here. :-)

[Sven Panne, 2012-08-17 09:30:12]

ARM and MIPS support complete, so the whole CL should be complete. :-) PTAL...

[Michael Starzinger, 2012-08-17 10:03:51]

LGTM (with a few final nits).

As discussed offline, it would be nice to trigger a deopt at the HCheckFunction
for the getter/setter, but I don't think our current type feedback through the
ICs is sufficient to actually trigger that.

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
File test/mjsunit/compiler/inline-accessors.js (right):

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:74: TestInlinedGetter(context, obj,
expected);
Can we add assertFalse(expectException) after the test?

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:150: TestGetterInAllContexts(getter4,
obj, undefined);
Can we add the fourth expectException argument here as well?

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:152: TestGetterInAllContexts(getter4,
obj, undefined);
Likewise.

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:234: TestInlinedSetter(context, obj,
value, expected);
Likewise.

[Sven Panne, 2012-08-17 11:33:49]

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
File test/mjsunit/compiler/inline-accessors.js (right):

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:74: TestInlinedGetter(context, obj,
expected);
On 2012/08/17 10:03:51, Michael Starzinger wrote:
> Can we add assertFalse(expectException) after the test?

Done.

https://chromiumcodereview.appspot.com/10855098/diff/6036/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:234: TestInlinedSetter(context, obj,
value, expected);
On 2012/08/17 10:03:51, Michael Starzinger wrote:
> Likewise.

Done.