To fully support hydrogen code stubs which accept a variable number of arguments,

To fully support hydrogen code stubs which accept a variable number of arguments,
the HReturn/LReturn instruction needs to be able to determine argument count
from a stack evaluation rather than as a constant from scope.

R=danno@chromium.org
BUG=

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

[mvstanton, 2013-03-07 13:30:13]

Hi Danno, here is the HReturn change.
As discussed, follow ups will be in deoptimizer support. I will await your
examination and then make the ports.
Thanks,
--Michael

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1959: : UseRegister(instr->parameter_count());
Hi Danno, is UseRegister the best predicate?

[danno, 2013-03-07 15:11:14]

Here ya go!

https://codereview.chromium.org/12613004/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12613004/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1483: HReturn(HValue* value, HValue* context, int
parameter_count) {
Do you really need this version? You can check in lithium if the value is a
constant, and it unifies this constructor with the other.

https://codereview.chromium.org/12613004/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1507: bool has_constant_parameter_count() const {
As noted above, you can determine this all in lithium-codegen-ia32.cc. If you
ask for a UseRegisterOrConstant in lithium-ia32.cc for this parameter, the
register allocation will choose a constant when it can, obviating the need to
track the "constantness" of the parameter count in hydrogen explicitly.

https://codereview.chromium.org/12613004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12613004/diff/1/src/hydrogen.cc#newcode3846
src/hydrogen.cc:3846: info()->num_parameters());
Create a HConstant with this value

https://codereview.chromium.org/12613004/diff/1/src/hydrogen.cc#newcode4733
src/hydrogen.cc:4733: info()->num_parameters()));
Create a HConstant with this value

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:2741: if (FLAG_debug_code) {
Can't you combine this code path with the one below?
You just need to insert an "__ inc(reg)" or use an adjusted parameter constant
count if dynamic_frame_alignment_ is true.

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:2767: if
(instr->has_constant_parameter_count()) {
if (instr->parameter_count()->IsConstantOperand())

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:2768: __ Ret((instr->constant_parameter_count()
+ 1) * kPointerSize, ecx);
int32_t parameter_count =
ToInteger32(LConstantOperand::cast(instr->parameter_count()));

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1959: : UseRegister(instr->parameter_count());
On 2013/03/07 13:30:13, mvstanton wrote:
> Hi Danno, is UseRegister the best predicate?

As commented elsewhere, UseRegisterOrConstant, but don't allow a NULL operand

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.h#newco...
src/ia32/lithium-ia32.h:1404: return hydrogen()->has_constant_parameter_count();
You can implement these in terms of calls on the parameter_count LOperand*,
maybe allowing you to keep some of the
has_constant_parameter_count/constant_parameter_count abstraction that you count
on in lithium-codegen-ia32.cc

[mvstanton, 2013-03-07 16:48:49]

Hi Danno, thanks for the good advice. It is cleaner now. All the best,
--Michael

https://codereview.chromium.org/12613004/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12613004/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1483: HReturn(HValue* value, HValue* context, int
parameter_count) {
On 2013/03/07 15:11:14, danno wrote:
> Do you really need this version? You can check in lithium if the value is a
> constant, and it unifies this constructor with the other.

Done.

https://codereview.chromium.org/12613004/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:1507: bool has_constant_parameter_count() const {
On 2013/03/07 15:11:14, danno wrote:
> As noted above, you can determine this all in lithium-codegen-ia32.cc. If you
> ask for a UseRegisterOrConstant in lithium-ia32.cc for this parameter, the
> register allocation will choose a constant when it can, obviating the need to
> track the "constantness" of the parameter count in hydrogen explicitly.

Done.

https://codereview.chromium.org/12613004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12613004/diff/1/src/hydrogen.cc#newcode3846
src/hydrogen.cc:3846: info()->num_parameters());
On 2013/03/07 15:11:14, danno wrote:
> Create a HConstant with this value

Done.

https://codereview.chromium.org/12613004/diff/1/src/hydrogen.cc#newcode4733
src/hydrogen.cc:4733: info()->num_parameters()));
On 2013/03/07 15:11:14, danno wrote:
> Create a HConstant with this value

Done.

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:2741: if (FLAG_debug_code) {
On 2013/03/07 15:11:14, danno wrote:
> Can't you combine this code path with the one below?
> You just need to insert an "__ inc(reg)" or use an adjusted parameter constant
> count if dynamic_frame_alignment_ is true.

Done.

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:2767: if
(instr->has_constant_parameter_count()) {
On 2013/03/07 15:11:14, danno wrote:
> if (instr->parameter_count()->IsConstantOperand())

Done.

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-codegen-ia32...
src/ia32/lithium-codegen-ia32.cc:2768: __ Ret((instr->constant_parameter_count()
+ 1) * kPointerSize, ecx);
On 2013/03/07 15:11:14, danno wrote:
> int32_t parameter_count =
> ToInteger32(LConstantOperand::cast(instr->parameter_count()));

Done.

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.cc#newc...
src/ia32/lithium-ia32.cc:1959: : UseRegister(instr->parameter_count());
On 2013/03/07 15:11:14, danno wrote:
> On 2013/03/07 13:30:13, mvstanton wrote:
> > Hi Danno, is UseRegister the best predicate?
> 
> As commented elsewhere, UseRegisterOrConstant, but don't allow a NULL operand

Done.

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/12613004/diff/1/src/ia32/lithium-ia32.h#newco...
src/ia32/lithium-ia32.h:1404: return hydrogen()->has_constant_parameter_count();
On 2013/03/07 15:11:14, danno wrote:
> You can implement these in terms of calls on the parameter_count LOperand*,
> maybe allowing you to keep some of the
> has_constant_parameter_count/constant_parameter_count abstraction that you
count
> on in lithium-codegen-ia32.cc

Done.

[mvstanton, 2013-03-08 13:21:44]

Hi Danno, I fixed that issue discussed earlier today in the checked in code
where the start environment is created twice, once in hydrogen.cc and once in
code-stubs-hydrogen.cc. Have a quick look. Thanks!
--Michael

[danno, 2013-03-08 15:10:04]

lgtm