Add inlining for intrinsics.

Add inlining for intrinsics.

This issue is for discussion on how to proceed.

I think the implementation of ValueOf shows that directly creating the IR does not scale.

BUG=
R=mstarzinger@chromium.org

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

[sigurds, 2014-09-29 12:06:37]

PTAL, but I'm looking for comments; I think this CL needs some time before it
can be landed.

[Michael Starzinger, 2014-10-15 08:45:00]

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-inlining...
src/compiler/js-inlining.cc:485: f->function_id != Runtime::kInlineValueOf) {
This only white-lists %_ValueOf to be inline-able. Is that intentional? If we
want to land it like this, then we should at least leave a fat TODO here.

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-intrinsi...
File src/compiler/js-intrinsic-builder.h (right):

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-intrinsi...
src/compiler/js-intrinsic-builder.h:19: std::pair<Node*, Node*>
BuildGraphFor(Runtime::FunctionId id,
Can we add a typedef for this pair?

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-intrinsi...
src/compiler/js-intrinsic-builder.h:22: std::pair<Node*, Node*>
BuildGraphFor_IsSmi(const NodeVector& arguments);
Can we make the dispatched methods private and only have one entry-point in this
builder for now?

[sigurds, 2014-10-15 12:39:35]

Adressed your comments.

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-inlining...
src/compiler/js-inlining.cc:485: f->function_id != Runtime::kInlineValueOf) {
On 2014/10/15 08:44:58, Michael Starzinger wrote:
> This only white-lists %_ValueOf to be inline-able. Is that intentional? If we
> want to land it like this, then we should at least leave a fat TODO here.

Leftover, cleaned it up.

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-intrinsi...
File src/compiler/js-intrinsic-builder.h (right):

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-intrinsi...
src/compiler/js-intrinsic-builder.h:19: std::pair<Node*, Node*>
BuildGraphFor(Runtime::FunctionId id,
On 2014/10/15 08:44:59, Michael Starzinger wrote:
> Can we add a typedef for this pair?

Done.

https://codereview.chromium.org/612043003/diff/80002/src/compiler/js-intrinsi...
src/compiler/js-intrinsic-builder.h:22: std::pair<Node*, Node*>
BuildGraphFor_IsSmi(const NodeVector& arguments);
On 2014/10/15 08:44:59, Michael Starzinger wrote:
> Can we make the dispatched methods private and only have one entry-point in
this
> builder for now?

Done.

[Michael Starzinger, 2014-10-15 13:03:18]

Looking good, just a couple of comments about the simplified lowering.

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:364: Node* is_smi =
jsgraph_->graph()->NewNode(
For posterity: There might be a more optimal lowering if the output
representation is determined to be kRepBit, but lets land it like this for now.

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:382: jsgraph_->machine()->Is32()
nit: There already should be IntLessThanOrEqual() shortcut for this.

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:872: return VisitIsSmi(node);
Please move this into the section for simplified operators, we are in the
section for machine operators here.

Also to be consistent with the rest of the code, the processing of inputs and
setting of output type (i.e. the "VisitFoo" method family) should be done
inline, whereas the actual lowering could be done in a helper (i.e. the "DoFoo"
method familiy).

[sigurds, 2014-10-15 14:59:59]

Adressed your comments. Will run bots once we have a good revision again.

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
File src/compiler/simplified-lowering.cc (right):

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:364: Node* is_smi =
jsgraph_->graph()->NewNode(
On 2014/10/15 13:03:17, Michael Starzinger wrote:
> For posterity: There might be a more optimal lowering if the output
> representation is determined to be kRepBit, but lets land it like this for
now.

Acknowledged.

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:382: jsgraph_->machine()->Is32()
On 2014/10/15 13:03:17, Michael Starzinger wrote:
> nit: There already should be IntLessThanOrEqual() shortcut for this.

Done.

https://codereview.chromium.org/612043003/diff/270001/src/compiler/simplified...
src/compiler/simplified-lowering.cc:872: return VisitIsSmi(node);
On 2014/10/15 13:03:17, Michael Starzinger wrote:
> Please move this into the section for simplified operators, we are in the
> section for machine operators here.
> 
> Also to be consistent with the rest of the code, the processing of inputs and
> setting of output type (i.e. the "VisitFoo" method family) should be done
> inline, whereas the actual lowering could be done in a helper (i.e. the
"DoFoo"
> method familiy).

Done. Did not create DoIsSmi/DoIsNonNegativeSmi.

[Michael Starzinger, 2014-10-17 09:58:51]

LGTM (with one last comment).

https://codereview.chromium.org/612043003/diff/330001/src/compiler/simplified...
File src/compiler/simplified-operator.h (right):

https://codereview.chromium.org/612043003/diff/330001/src/compiler/simplified...
src/compiler/simplified-operator.h:158: const Operator* IsSmi();
nit: Benedikt suggested to call these operators "ObjectIsSmi" and
"ObjectIsNonNegativeSmi" instead. I agree, this makes it clear what the input
type is. Also we should move them up a few lines to the other pure operators
(i.e. to line 139).

[sigurds, 2014-10-20 07:57:14]

Committed patchset #10 (id:370001) manually as 24719 (presubmit successful).

[sigurds, 2014-10-21 14:42:25]

https://codereview.chromium.org/612043003/diff/330001/src/compiler/simplified...
File src/compiler/simplified-operator.h (right):

https://codereview.chromium.org/612043003/diff/330001/src/compiler/simplified...
src/compiler/simplified-operator.h:158: const Operator* IsSmi();
On 2014/10/17 09:58:51, Michael Starzinger wrote:
> nit: Benedikt suggested to call these operators "ObjectIsSmi" and
> "ObjectIsNonNegativeSmi" instead. I agree, this makes it clear what the input
> type is. Also we should move them up a few lines to the other pure operators
> (i.e. to line 139).

Done.