Reland arguments access support for inlined functions (r11109,r11118).

Reland arguments access support for inlined functions (r11109,r11118).

When pushing arguments use correct initial values instead of fetching them from the environment which can be modified.

R=fschneider@chromium.org
TEST=test/mjsunit/compiler/inline-arguments.js

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

[Kevin Millikin (Chromium), 2012-04-11 11:49:18]

I have issues.

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.cc#newcode...
src/arm/lithium-arm.cc:1081: current_block_->last_environment()->outer() !=
NULL));
I'd be more comfortable if we set this flag on the Hydrogen instruction instead.

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.h#newcode1388
src/arm/lithium-arm.h:1388: class LPop: public LTemplateInstruction<0, 0, 0> {
I think we normally use the terminology "Drop" for something that can pop more
than one and "Pop" for popping exactly one.

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-codegen-arm.cc...
src/arm/lithium-codegen-arm.cc:2768: __ add(result, sp, Operand(-2 *
kPointerSize));
add?  sub?

I think it's weird to have this off-by-two in both the inlined and non-inlined
cases.  For the inlined case there is a sub here and then an add at every
element access to compensate.  For the non-inlined case there is an add at every
element access instead of just having one here.

I guess the reason is that ArgumentsLength would then have to change, but the
way we do that seems like it could be better, too.  (Setting a register to
either fp or not fp, then comparing it to fp to see if we set it to fp or not
fp, then issuing a duplicate memory load of caller fp if we have an adaptor....)

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.cc#newcode5005
src/hydrogen.cc:5005: if (!function_state()->arguments_pushed()) {
I'd move this after all possible "return false", so that the graph is not
mutated in the case we return false.

That might require duplicating the code.  If so, factor it out into a separate
function.

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.h#newcode757
src/hydrogen.h:757: HInstruction* entry_;
Why not HEnterInlined* and HArgumentsElements* below?

[Vyacheslav Egorov (Chromium), 2012-04-11 12:47:28]

PTAL

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.cc#newcode...
src/arm/lithium-arm.cc:1081: current_block_->last_environment()->outer() !=
NULL));
On 2012/04/11 11:49:18, Kevin Millikin wrote:
> I'd be more comfortable if we set this flag on the Hydrogen instruction
instead.

Done.

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-arm.h#newcode1388
src/arm/lithium-arm.h:1388: class LPop: public LTemplateInstruction<0, 0, 0> {
On 2012/04/11 11:49:18, Kevin Millikin wrote:
> I think we normally use the terminology "Drop" for something that can pop more
> than one and "Pop" for popping exactly one.

Done.

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):

http://codereview.chromium.org/10033028/diff/1/src/arm/lithium-codegen-arm.cc...
src/arm/lithium-codegen-arm.cc:2768: __ add(result, sp, Operand(-2 *
kPointerSize));
Off by one is there to avoid changing access code.

On 2012/04/11 11:49:18, Kevin Millikin wrote:
> add?  sub?
> 
> I think it's weird to have this off-by-two in both the inlined and non-inlined
> cases.  For the inlined case there is a sub here and then an add at every
> element access to compensate.  For the non-inlined case there is an add at
every
> element access instead of just having one here.
> 
> I guess the reason is that ArgumentsLength would then have to change, but the
> way we do that seems like it could be better, too.  (Setting a register to
> either fp or not fp, then comparing it to fp to see if we set it to fp or not
> fp, then issuing a duplicate memory load of caller fp if we have an
adaptor....)

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.cc#newcode5005
src/hydrogen.cc:5005: if (!function_state()->arguments_pushed()) {
On 2012/04/11 11:49:18, Kevin Millikin wrote:
> I'd move this after all possible "return false", so that the graph is not
> mutated in the case we return false.
> 
> That might require duplicating the code.  If so, factor it out into a separate
> function.

Done.

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/10033028/diff/1/src/hydrogen.h#newcode757
src/hydrogen.h:757: HInstruction* entry_;
On 2012/04/11 11:49:18, Kevin Millikin wrote:
> Why not HEnterInlined* and HArgumentsElements* below?

Done.

[Kevin Millikin (Chromium), 2012-04-11 12:54:41]

LGTM.

http://codereview.chromium.org/10033028/diff/8001/src/arm/lithium-arm.h
File src/arm/lithium-arm.h (right):

http://codereview.chromium.org/10033028/diff/8001/src/arm/lithium-arm.h#newco...
src/arm/lithium-arm.h:538: explicit LArgumentsElements(bool from_inlined)
The flag could be gotten from the hydrogen_value().