Deoptimizer support for hydrogen stubs that accept a variable number of arguments.

Deoptimizer support for hydrogen stubs that accept a variable number of arguments.

BUG=

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

[mvstanton, 2013-03-13 12:57:59]

Hi Danno,
Here is the 2nd part of the deoptimizer and graph generator support for variable
argument stubs, needed by the array constructor stubs to be checked in soon.
Thanks for having a look!
--Michael

[danno, 2013-03-20 21:07:32]

here you go...

https://codereview.chromium.org/12490013/diff/2001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12490013/diff/2001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:127: HInstruction* stack_pop_count;
nit: Can you define this down where you initially set its value?

https://codereview.chromium.org/12490013/diff/2001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/12490013/diff/2001/src/deoptimizer.cc#newcode...
src/deoptimizer.cc:1526: bool is_native_value) {
I think an enum is better here:
enum DeoptimizerTranslatedValueType {
  TRANSLATED_VALUE_IS_NATIVE,
  TRANSLATED_VALUE_IS_TAGGED,
}

https://codereview.chromium.org/12490013/diff/2001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/12490013/diff/2001/src/frames.cc#newcode1317
src/frames.cc:1317: bool acting_as_js_function = (i == 0) ? false : true;
Since there is only true/false, I think removing the loop is better and just
testing the two versions linearly makes more sense.

https://codereview.chromium.org/12490013/diff/2001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/12490013/diff/2001/src/objects-inl.h#newcode3846
src/objects-inl.h:3846: ASSERT(kind() == OPTIMIZED_FUNCTION || kind() ==
COMPILED_STUB);
Why is this additional check needed?

[mvstanton, 2013-03-21 11:47:35]

Hi Danno, addressed comments, thanks very much.
Sorry, my upload includes a rebase, so the diffs may be noisy, but there isn't
too much to look at.
--Michael

https://codereview.chromium.org/12490013/diff/2001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12490013/diff/2001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:127: HInstruction* stack_pop_count;
On 2013/03/20 21:07:33, danno wrote:
> nit: Can you define this down where you initially set its value?

Done.

https://codereview.chromium.org/12490013/diff/2001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/12490013/diff/2001/src/deoptimizer.cc#newcode...
src/deoptimizer.cc:1526: bool is_native_value) {
On 2013/03/20 21:07:33, danno wrote:
> I think an enum is better here:
> enum DeoptimizerTranslatedValueType {
>   TRANSLATED_VALUE_IS_NATIVE,
>   TRANSLATED_VALUE_IS_TAGGED,
> }

Done, though I kept a bool local is_native for the TraceValueType helper as this
was easier.

https://codereview.chromium.org/12490013/diff/2001/src/frames.cc
File src/frames.cc (right):

https://codereview.chromium.org/12490013/diff/2001/src/frames.cc#newcode1317
src/frames.cc:1317: bool acting_as_js_function = (i == 0) ? false : true;
On 2013/03/20 21:07:33, danno wrote:
> Since there is only true/false, I think removing the loop is better and just
> testing the two versions linearly makes more sense.

Thanks, I thought this was awkward too, whew.

https://codereview.chromium.org/12490013/diff/2001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/12490013/diff/2001/src/objects-inl.h#newcode3846
src/objects-inl.h:3846: ASSERT(kind() == OPTIMIZED_FUNCTION || kind() ==
COMPILED_STUB);
On 2013/03/20 21:07:33, danno wrote:
> Why is this additional check needed?

I removed it, and spoke with Ulan. In fact we should prevent compiled stubs from
getting into a list of code that is checked for this flag. A separate change in
LCodeGen::FinishCode() will guard against anything other than
OPTIMIZED_FUNCTIONs getting in there.

[danno, 2013-03-22 14:06:49]

https://codereview.chromium.org/12490013/diff/4002/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12490013/diff/4002/src/code-stubs.h#newcode270
src/code-stubs.h:270: bool acting_as_js_function_;
Can you please turn this bool into an enum?

https://codereview.chromium.org/12490013/diff/4002/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/12490013/diff/4002/src/deoptimizer.cc#newcode...
src/deoptimizer.cc:1502: static const char* TraceValueType(bool is_smi, bool
is_native) {
Any reason that you don't pass a DeoptimizerTranslatedValueType here?

[mvstanton, 2013-04-02 07:43:12]

Hi Danno, thanks I've addressed those points, --Michael

https://codereview.chromium.org/12490013/diff/4002/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12490013/diff/4002/src/code-stubs.h#newcode270
src/code-stubs.h:270: bool acting_as_js_function_;
On 2013/03/22 14:06:50, danno wrote:
> Can you please turn this bool into an enum?

Done.

https://codereview.chromium.org/12490013/diff/4002/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/12490013/diff/4002/src/deoptimizer.cc#newcode...
src/deoptimizer.cc:1502: static const char* TraceValueType(bool is_smi, bool
is_native) {
On 2013/03/22 14:06:50, danno wrote:
> Any reason that you don't pass a DeoptimizerTranslatedValueType here?

I did agonize about that one for a bit.
The first problem is that I'd have to make the enum public (or make this
printing helper a class member). The second is that we also have this bool
is_smi case, and I didn't like the asymmetrical feel of the function if it took
one bool and one enum. So I think locally it is better if the bool is used.

[danno, 2013-04-02 10:39:44]

lgtm with comments

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:143: HInstruction* stack_pop_count =
stack_parameter_count;
nit: can you please move this right before the if which has the alternative
definition? Puts it closer to the code it's related to.

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs.h#newcode262
src/code-stubs.h:262: enum StubFunctionMode { NOT_JS_FUNCTION_MODE,
JS_FUNCTION_MODE };
How about NOT_JS_FUNCTION_STUB_MODE and JS_FUNCTION_STUB_MODE? Makes it a bit
clear and less ambiguous in the global namespace.

https://codereview.chromium.org/12490013/diff/16001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/12490013/diff/16001/src/deoptimizer.cc#newcod...
src/deoptimizer.cc:1582: // We save the untagged value on the side and store a
GC-safe
Here and below:

ASSERT here that value_type == TRANSLATED_VALUE_IS_TAGGED?

[mvstanton, 2013-04-02 11:27:25]

Thanks, I addressed those points. Much appreciated!
--Michael

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:143: HInstruction* stack_pop_count =
stack_parameter_count;
On 2013/04/02 10:39:44, danno wrote:
> nit: can you please move this right before the if which has the alternative
> definition? Puts it closer to the code it's related to.

Done.

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/12490013/diff/16001/src/code-stubs.h#newcode262
src/code-stubs.h:262: enum StubFunctionMode { NOT_JS_FUNCTION_MODE,
JS_FUNCTION_MODE };
On 2013/04/02 10:39:44, danno wrote:
> How about NOT_JS_FUNCTION_STUB_MODE and JS_FUNCTION_STUB_MODE? Makes it a bit
> clear and less ambiguous in the global namespace.

Done.

https://codereview.chromium.org/12490013/diff/16001/src/deoptimizer.cc
File src/deoptimizer.cc (right):

https://codereview.chromium.org/12490013/diff/16001/src/deoptimizer.cc#newcod...
src/deoptimizer.cc:1582: // We save the untagged value on the side and store a
GC-safe
On 2013/04/02 10:39:44, danno wrote:
> Here and below:
> 
> ASSERT here that value_type == TRANSLATED_VALUE_IS_TAGGED?

Thanks, good idea. Done.

[mvstanton, 2013-04-02 11:28:18]

Committed patchset #6 manually as r14111 (presubmit successful).