Refactor optimized in hydrogen only runtime functions.

Refactor optimized in hydrogen only runtime functions.

This splits all runtime function into 3 categories:
1) RUNTIME: implemented in runtime and called from both full and optimized code.
2) RUNTIME_HIDDEN: implemented in runtime, never called directly from JS builtins.
3) INLINE: inlined in both full and optimized code
4) INLINE_OPTIMIZED: inlined in optimized code, implemented in runtime for full code.

R=yangguo@chromium.org, yannguo@chromium.org

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

[Dmitry Lomov (no reviews), 2014-03-24 12:45:17]

PTAL

[Dmitry Lomov, 2014-03-24 12:54:52]

PTAL

[Yang, 2014-03-24 14:04:02]

Goes in the right direction, but I got some suggestions.

https://codereview.chromium.org/209353006/diff/10001/src/runtime.h
File src/runtime.h (left):

https://codereview.chromium.org/209353006/diff/10001/src/runtime.h#oldcode649
src/runtime.h:649: #define INLINE_RUNTIME_FUNCTION_LIST(F) \
Entries in this list are expanded to kFoo and kInlineFoo. There is no need to
remove this list and add duplicates called kFooInternal to RUNTIME_FUNCTION_LIST
and rename all the uses.

How about keeping this list, and put functions that in their inline version only
call the runtime version into the new INLINE_OPTIMIZED_FUNCTION_LIST?

If you insist in calling them kFooInternal, I suggest using the macro to expand
the definitions instead of adding those duplicates all over
RUNTIME_FUNCTION_LIST.

Either way, I suggest adding another intrinsic type (RUNTIME_INTERNAL or
RUNTIME_HIDDEN) for those runtime functions that are not supposed to be called
directly and only act as slow case for %_Foo and make sure they are not callable
through %Foo from anywhere.

[Dmitry Lomov (no reviews), 2014-03-24 14:39:05]

On 2014/03/24 14:04:02, Yang wrote:
> Goes in the right direction, but I got some suggestions.
> 
> https://codereview.chromium.org/209353006/diff/10001/src/runtime.h
> File src/runtime.h (left):
> 
> https://codereview.chromium.org/209353006/diff/10001/src/runtime.h#oldcode649
> src/runtime.h:649: #define INLINE_RUNTIME_FUNCTION_LIST(F) \
> Entries in this list are expanded to kFoo and kInlineFoo. There is no need to
> remove this list and add duplicates called kFooInternal to
RUNTIME_FUNCTION_LIST
> and rename all the uses.
> 
> How about keeping this list, and put functions that in their inline version
only
> call the runtime version into the new INLINE_OPTIMIZED_FUNCTION_LIST?
> 
> If you insist in calling them kFooInternal, I suggest using the macro to
expand
> the definitions instead of adding those duplicates all over
> RUNTIME_FUNCTION_LIST.

I am not sure I follow you here: kFooInternal are very different from
k(Inline)Foo. 
We should not mix them.

> 
> Either way, I suggest adding another intrinsic type (RUNTIME_INTERNAL or
> RUNTIME_HIDDEN) for those runtime functions that are not supposed to be called
> directly and only act as slow case for %_Foo and make sure they are not
callable
> through %Foo from anywhere.

This is a good idea - will do.

[Dmitry Lomov (no reviews), 2014-03-24 16:02:33]

Comments addressed - PTAL

[Yang, 2014-03-25 13:19:59]

LGTM if comments addressed.

It would be nice to have a test ensuring we cannot call %HiddenFoo.

https://codereview.chromium.org/209353006/diff/30001/src/ia32/full-codegen-ia...
File src/ia32/full-codegen-ia32.cc (right):

https://codereview.chromium.org/209353006/diff/30001/src/ia32/full-codegen-ia...
src/ia32/full-codegen-ia32.cc:4147: expr->function()->intrinsic_type ==
Runtime::INLINE ) {
stray white space after Runtime::INLINE.

https://codereview.chromium.org/209353006/diff/30001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/209353006/diff/30001/src/runtime.cc#newcode14604
src/runtime.cc:14604: // Exclude the code in release mode.
Would be nice to update this as well.

https://codereview.chromium.org/209353006/diff/30001/src/runtime.h
File src/runtime.h (right):

https://codereview.chromium.org/209353006/diff/30001/src/runtime.h#newcode605
src/runtime.h:605: // RUNTIME_HIDDEN_FUNCTION_LIST defined all runtime functions
accessed
s/defined/defines/

https://codereview.chromium.org/209353006/diff/30001/test/mjsunit/fuzz-native...
File test/mjsunit/fuzz-natives-part1.js (right):

https://codereview.chromium.org/209353006/diff/30001/test/mjsunit/fuzz-native...
test/mjsunit/fuzz-natives-part1.js:164: "LogInternal": true,
Outdated name? Do we even need this line? We should add all hidden runtime
functions to %ListNatives, and make sure that calling them in this test will not
crash (simply throws in an eval).

[Dmitry Lomov, 2014-03-25 13:57:16]

Comments addressed, landing.

[Dmitry Lomov (no reviews), 2014-03-25 14:27:15]

Committed patchset #3 manually as r20252 (presubmit successful).

[Sven Panne, 2014-03-26 08:47:14]

On 2014/03/25 14:27:15, Dmitry Lomov (chromium) wrote:
> Committed patchset #3 manually as r20252 (presubmit successful).

This breaks compilation with regexp=interpreted.