Tweak the TurboFan pipeline for stub compilation.

Tweak the TurboFan pipeline for stub compilation.

Committed: https://crrev.com/9118b2fa87421c0cc23430eae8f5f9465ccb6b17
Cr-Commit-Position: refs/heads/master@{#27268}

[Sven Panne, 2015-03-17 09:05:19]

svenpanne@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Michael Starzinger, 2015-03-17 17:28:56]

Looking good, just nits.

https://codereview.chromium.org/1010673004/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1010673004/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:319: bool CreateGraph(bool constant_context) {
nit: Can we add a "bool stack_check" to the CreateGraph method here and move the
"info->IsStub()" to the call-site? That would make it less hidden away.

https://codereview.chromium.org/1010673004/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:336: bool stack_check_;
nit: This field is not required once the above comment is addressed.

https://codereview.chromium.org/1010673004/diff/60001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1010673004/diff/60001/src/runtime.js#newcode698
src/runtime.js:698: 
nit: Can we put in a separator comment here like the following? This would make
it clear what these functions are intended for.

/* -----------------------------------------------
   - - -   J a v a S c r i p t   S t u b s   - - -
   -----------------------------------------------
*/

https://codereview.chromium.org/1010673004/diff/60001/src/runtime.js#newcode699
src/runtime.js:699: function StringLengthStub(name) {
nit: Also maybe make this one function all-uppercase, so that the comment in
lines 7 to 9 applies.

[Sven Panne, 2015-03-18 08:45:55]

Feedback addressed, PTAL again...

https://codereview.chromium.org/1010673004/diff/60001/src/compiler/pipeline.cc
File src/compiler/pipeline.cc (right):

https://codereview.chromium.org/1010673004/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:319: bool CreateGraph(bool constant_context) {
On 2015/03/17 17:28:55, Michael Starzinger wrote:
> nit: Can we add a "bool stack_check" to the CreateGraph method here and move
the
> "info->IsStub()" to the call-site? That would make it less hidden away.

Done.

https://codereview.chromium.org/1010673004/diff/60001/src/compiler/pipeline.c...
src/compiler/pipeline.cc:336: bool stack_check_;
On 2015/03/17 17:28:55, Michael Starzinger wrote:
> nit: This field is not required once the above comment is addressed.

Done.

https://codereview.chromium.org/1010673004/diff/60001/src/runtime.js
File src/runtime.js (right):

https://codereview.chromium.org/1010673004/diff/60001/src/runtime.js#newcode698
src/runtime.js:698: 
On 2015/03/17 17:28:55, Michael Starzinger wrote:
> nit: Can we put in a separator comment here like the following? This would
make
> it clear what these functions are intended for.
> 
> /* -----------------------------------------------
>    - - -   J a v a S c r i p t   S t u b s   - - -
>    -----------------------------------------------
> */

Done.

https://codereview.chromium.org/1010673004/diff/60001/src/runtime.js#newcode699
src/runtime.js:699: function StringLengthStub(name) {
On 2015/03/17 17:28:55, Michael Starzinger wrote:
> nit: Also maybe make this one function all-uppercase, so that the comment in
> lines 7 to 9 applies.

Done.

[Michael Starzinger, 2015-03-18 09:03:46]

LGTM from my end.

[titzer, 2015-03-18 09:25:51]

titzer@chromium.org changed reviewers:
+ titzer@chromium.org

[titzer, 2015-03-18 09:25:53]

https://codereview.chromium.org/1010673004/diff/80001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1010673004/diff/80001/src/compiler.cc#newcode224
src/compiler.cc:224: return scope() == nullptr ? parameter_count_ :
scope()->num_parameters();
Maybe we should have a has_scope() predicate?

https://codereview.chromium.org/1010673004/diff/80001/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1010673004/diff/80001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:118:
NodeProperties::ReplaceWithValue(node, result);
This looks like a good candidate to mutate in place. You should be able to just
do:

NodeProperties::ReplaceWithValue(node, node)

https://codereview.chromium.org/1010673004/diff/80001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:205:
NodeProperties::ReplaceWithValue(node, result);
Same as above.

https://codereview.chromium.org/1010673004/diff/80001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/1010673004/diff/80001/src/hydrogen.h#newcode2237
src/hydrogen.h:2237: /* JSValue */                        \
We need a better name for this operation.

https://codereview.chromium.org/1010673004/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1010673004/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:720: F(JSValueValue, 1, 1)
We need a better name somehow...

BTW what are the checks associated with this JSValueValue? What happens if you
pass a SMI?

[Sven Panne, 2015-03-18 09:34:47]

The CQ bit was checked by svenpanne@chromium.org

[commit-bot: I haz the power, 2015-03-18 09:34:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1010673004/80001

[Sven Panne, 2015-03-18 09:35:22]

The CQ bit was unchecked by svenpanne@chromium.org

[Sven Panne, 2015-03-18 10:40:09]

https://codereview.chromium.org/1010673004/diff/80001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1010673004/diff/80001/src/compiler.cc#newcode224
src/compiler.cc:224: return scope() == nullptr ? parameter_count_ :
scope()->num_parameters();
On 2015/03/18 09:25:53, titzer wrote:
> Maybe we should have a has_scope() predicate?

Done.

https://codereview.chromium.org/1010673004/diff/80001/src/compiler/js-intrins...
File src/compiler/js-intrinsic-lowering.cc (right):

https://codereview.chromium.org/1010673004/diff/80001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:118:
NodeProperties::ReplaceWithValue(node, result);
On 2015/03/18 09:25:53, titzer wrote:
> This looks like a good candidate to mutate in place. You should be able to
just
> do:
> 
> NodeProperties::ReplaceWithValue(node, node)
> 

Done.

https://codereview.chromium.org/1010673004/diff/80001/src/compiler/js-intrins...
src/compiler/js-intrinsic-lowering.cc:205:
NodeProperties::ReplaceWithValue(node, result);
On 2015/03/18 09:25:53, titzer wrote:
> Same as above.

Done.

https://codereview.chromium.org/1010673004/diff/80001/src/runtime/runtime.h
File src/runtime/runtime.h (right):

https://codereview.chromium.org/1010673004/diff/80001/src/runtime/runtime.h#n...
src/runtime/runtime.h:720: F(JSValueValue, 1, 1)
On 2015/03/18 09:25:53, titzer wrote:
> We need a better name somehow...

Done.

> BTW what are the checks associated with this JSValueValue? What happens if you
> pass a SMI?

As discussed offline: There are intentionally no checks, all intrinsics are
unsafe by definition. What we probably should do in the future is to have some
kind of type annotation which can be checked in debug builds.

[Sven Panne, 2015-03-18 10:40:33]

The CQ bit was checked by svenpanne@chromium.org

[Sven Panne, 2015-03-18 10:40:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1010673004/#ps100001
(title: "Addressed feedback")

[commit-bot: I haz the power, 2015-03-18 10:40:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1010673004/100001

[commit-bot: I haz the power, 2015-03-18 11:42:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-18 11:42:53]

Patchset 6 (id:??) landed as
https://crrev.com/9118b2fa87421c0cc23430eae8f5f9465ccb6b17
Cr-Commit-Position: refs/heads/master@{#27268}