Inline simple setter calls.

Inline simple setter calls.

Currently only simple setter calls are handled (i.e. no calls in count
operations or compound assignments), and deoptimization in the setter is not
handled at all. Because of the latter, we temporarily hide this feature behind
the --inline-accessors flag, just like inlining getters.

We now use an enum everywhere we depend on the handling of a return value,
passing around several boolean would be more confusing.

Made VisitReturnStatement and the final parts of TryInline more similar, so
matching them visually is a bit easier now.

Simplified the signature of AddLeaveInlined, the target of the HGoto can simply
be retrieved from the function state.

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

[Michael Starzinger, 2012-08-08 13:05:33]

First round.

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:6876: // Falling from the end of an inlined construct call. In a
test context
Hmm, how about "Falling off the end ..."?

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:6888: // Falling from the end of an inlined setter call. The
returned value is
Likewise.

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:6900: // Falling from the end of a normal inlined function. This
basically means
Likewise.

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:9274: ReturnHandlingFlag return_handling) const {
Since this flag is not only used to distinguish the return handling, but also
the entry-kind for inlining, I would suggest to rename it to "InlininigKind" or
something like that.

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

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:33: function Constr3() { }
Can we move the constructors down, right before they are actually used?

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:81: return 1234;
You might want to consider returning a global "getterValue" here, and combine it
with some addition that can be sued to cause deopts once you start working on
the deoptimizer.

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:86:
TestGetterInAllContexts(Object.create(obj));
Also test a getter that falls off the function end without returning anything.

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:113: return obj.setterProperty = value
+ 1;
I wouldn't use an addition on the RHS, otherwise it will deopt here, instead of
inside the setter.

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:153:
TestSetterInAllContexts(Object.create(obj));
We also need tests for arguments mismatch of both setters and getters. For the
getter that is too many arguments. For the setter that is both, too many and too
few arguments.

[Sven Panne, 2012-08-08 14:15:20]

Incorporated most comments, I'll have to investigate a failed ASSERT before
writing more tests... :-P

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:6876: // Falling from the end of an inlined construct call. In a
test context
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> Hmm, how about "Falling off the end ..."?

Done.

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:6888: // Falling from the end of an inlined setter call. The
returned value is
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:6900: // Falling from the end of a normal inlined function. This
basically means
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/10836133/diff/4003/src/hydrogen.cc#new...
src/hydrogen.cc:9274: ReturnHandlingFlag return_handling) const {
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> Since this flag is not only used to distinguish the return handling, but also
> the entry-kind for inlining, I would suggest to rename it to "InlininigKind"
or
> something like that.

Done.

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

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:33: function Constr3() { }
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> Can we move the constructors down, right before they are actually used?

Done.

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:81: return 1234;
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> You might want to consider returning a global "getterValue" here, and combine
it
> with some addition that can be sued to cause deopts once you start working on
> the deoptimizer.

As discussed offline, this will be done via an explicit "expected" argument plus
a global variable for forcing deopts.

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:86:
TestGetterInAllContexts(Object.create(obj));
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> Also test a getter that falls off the function end without returning anything.

Done.

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:113: return obj.setterProperty = value
+ 1;
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> I wouldn't use an addition on the RHS, otherwise it will deopt here, instead
of
> inside the setter.

Done.

https://chromiumcodereview.appspot.com/10836133/diff/4003/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:153:
TestSetterInAllContexts(Object.create(obj));
On 2012/08/08 13:05:33, Michael Starzinger wrote:
> We also need tests for arguments mismatch of both setters and getters. For the
> getter that is too many arguments. For the setter that is both, too many and
too
> few arguments.

Still a TODO...

[Michael Starzinger, 2012-08-08 15:01:29]

One round of nits, otherwise it's looking good. I'll hold off with the final
ElGeTeEm until all tests pass though.

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.cc#new...
src/hydrogen.cc:188: AddInstruction(new(zone())
HLeaveInlined(state->arguments_pushed()));
Can we keep the drop_extra and arguments_pushed as local variables at the top,
IMHO that's cleaner. And maybe also move the target block into a separate local
variable.

HBasicBlock* block = state->function_return();

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.cc#new...
src/hydrogen.cc:6969: receiver,
Can we also rename "receiver" to "implicit_return_value" here for consistency?

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.h
File src/hydrogen.h (right):

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.h#newc...
src/hydrogen.h:745: InliningKind inlining_kind_;
Can we have a one-liner comment that says this specifies the kind of call that
was inlined.

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

https://chromiumcodereview.appspot.com/10836133/diff/8017/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:79: function getter1() {
It might be a good idea to assert that the receiver is passed correctly in all
of the getters and setters. Something like the following ...

assertSame(obj, this);

[Sven Panne, 2012-08-09 12:15:32]

Incorporated comments and extended test. Now hunting a bug related to a no-arg
setter... :-P

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.cc#new...
src/hydrogen.cc:188: AddInstruction(new(zone())
HLeaveInlined(state->arguments_pushed()));
On 2012/08/08 15:01:29, Michael Starzinger wrote:
> Can we keep the drop_extra and arguments_pushed as local variables at the top,
> IMHO that's cleaner. And maybe also move the target block into a separate
local
> variable.
> 
> HBasicBlock* block = state->function_return();

Done.

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.cc#new...
src/hydrogen.cc:6969: receiver,
On 2012/08/08 15:01:29, Michael Starzinger wrote:
> Can we also rename "receiver" to "implicit_return_value" here for consistency?

Done.

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.h
File src/hydrogen.h (right):

https://chromiumcodereview.appspot.com/10836133/diff/8017/src/hydrogen.h#newc...
src/hydrogen.h:745: InliningKind inlining_kind_;
On 2012/08/08 15:01:29, Michael Starzinger wrote:
> Can we have a one-liner comment that says this specifies the kind of call that
> was inlined.

Done.

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

https://chromiumcodereview.appspot.com/10836133/diff/8017/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:79: function getter1() {
On 2012/08/08 15:01:29, Michael Starzinger wrote:
> It might be a good idea to assert that the receiver is passed correctly in all
> of the getters and setters. Something like the following ...
> 
> assertSame(obj, this);

Done.

[Michael Starzinger, 2012-08-09 12:20:55]

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

https://chromiumcodereview.appspot.com/10836133/diff/4006/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:80: assertEquals(obj, this);
I would really use assertSame() here instead, because you want identity
comparison and not a deepEquals() of the object body.

[Sven Panne, 2012-08-09 14:06:37]

Done. Extended and fixed version ready for next round of review... :-)

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

https://chromiumcodereview.appspot.com/10836133/diff/4006/test/mjsunit/compil...
test/mjsunit/compiler/inline-accessors.js:80: assertEquals(obj, this);
On 2012/08/09 12:20:55, Michael Starzinger wrote:
> I would really use assertSame() here instead, because you want identity
> comparison and not a deepEquals() of the object body.

Done.

[Michael Starzinger, 2012-08-10 08:52:15]

LGTM (with one nit and one important follow-up for the next CL).

https://chromiumcodereview.appspot.com/10836133/diff/9004/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10836133/diff/9004/src/hydrogen.cc#new...
src/hydrogen.cc:5248: Drop(2);
It seems dropping the arguments after doing HChceckConstantFunction is the
correct order, so it was wrong before. This proves that we don't have test
coverage for that and need to add tests. But we can do that in a follow-up CL.

https://chromiumcodereview.appspot.com/10836133/diff/9004/src/hydrogen.cc#new...
src/hydrogen.cc:9299: outer = CreateStubEnvironment(outer, target, JS_SETTER,
arguments);
Can we add a comment specifying which stub is simulated by this artificial
environment?