Add initial support for inlining.

Add initial support for inlining.

* Add stack depth checking to function tester.
* Make context specialization clean up after itself.
* Add UpdateToAndIncrement to Inputs::iterator.
  Uses:iterator already provides this member function.
* Allow next node id in graph to be set.

R=mstarzinger@chromium.org, titzer@chromium.org

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

[Michael Starzinger, 2014-08-11 18:42:05]

Can you please re-upload so that it doesn't include CL 462633003, otherwise it
is hard to review. Also v8-dev@ should be CCed on all CLs so that everyone has a
chance to comment if something jumps out.

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

https://codereview.chromium.org/453833003/diff/1/src/compiler/js-inlining.cc#...
src/compiler/js-inlining.cc:5: #include "js-inlining.h"
nit: Use full include path here.

https://codereview.chromium.org/453833003/diff/1/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/453833003/diff/1/test/cctest/cctest.gyp#newco...
test/cctest/cctest.gyp:77: 'compiler/test-run-inlining.cc',
nit: Please keep alpha-sorted.

[sigurds, 2014-08-12 12:29:30]

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

https://codereview.chromium.org/453833003/diff/1/src/compiler/js-inlining.cc#...
src/compiler/js-inlining.cc:5: #include "js-inlining.h"
On 2014/08/11 18:42:04, Michael Starzinger wrote:
> nit: Use full include path here.

Done.

https://codereview.chromium.org/453833003/diff/1/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/453833003/diff/1/test/cctest/cctest.gyp#newco...
test/cctest/cctest.gyp:77: 'compiler/test-run-inlining.cc',
On 2014/08/11 18:42:04, Michael Starzinger wrote:
> nit: Please keep alpha-sorted.

Done.

[sigurds, 2014-08-12 12:30:31]

On 2014/08/12 12:29:30, sigurds wrote:
> https://codereview.chromium.org/453833003/diff/1/src/compiler/js-inlining.cc
> File src/compiler/js-inlining.cc (right):
> 
>
https://codereview.chromium.org/453833003/diff/1/src/compiler/js-inlining.cc#...
> src/compiler/js-inlining.cc:5: #include "js-inlining.h"
> On 2014/08/11 18:42:04, Michael Starzinger wrote:
> > nit: Use full include path here.
> 
> Done.
> 
> https://codereview.chromium.org/453833003/diff/1/test/cctest/cctest.gyp
> File test/cctest/cctest.gyp (right):
> 
>
https://codereview.chromium.org/453833003/diff/1/test/cctest/cctest.gyp#newco...
> test/cctest/cctest.gyp:77: 'compiler/test-run-inlining.cc',
> On 2014/08/11 18:42:04, Michael Starzinger wrote:
> > nit: Please keep alpha-sorted.
> 
> Done.

Does not contain CL 462633003 anymore and v8-dev@ is CC'd.

[Michael Starzinger, 2014-08-12 20:04:47]

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

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:75: /// Ensure that only a single return reaches the
end node.
nit: Two slashes ought to be enough for everyone. :)

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:121: void moveWithDependencies(JSGraph* graph, Node*
node, Node* old_block,
style: Uppercase "M". Also needs to be static.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:155: static void moveAllControlNodes(Node* from,
Node* to) {
style: Uppercase "M".

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:166: void DoInline(Zone* zone, Node* call,
CompilationInfo* info, JSGraph* jsgraph) {
Either make this a (private) member method of JSInliner instead of passing
everything around as arguments or at least make this static.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:237: if (IsStackGuard(stackGuard)) {
Let's just drop the removal of the stack-guard for now. This will break once we
implement actual stack-guards (instead of runtime calls) and should not affect
correctness.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:268: SmartArrayPointer<char> name =
function->shared()->DebugName()->ToCString();
nit: The "name" is only use for tracing, please move into the tracing blocks
(even though it's duplicated).

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:272: printf("Not Inlining %s into %s because inlinee
is native\n", name.get(),
nit: s/printf/PrintF/

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:291: printf(
nit: s/printf/PrintF/

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:304: printf("Inlining %s into %s\n", name.get(),
nit: s/printf/PrintF/

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:309:
graph.SetNextNodeId(jsgraph_->graph()->NodeCount());
I assume this is only temporary for now that we don't copy the nodes from the
inlinee but rather create the nodes in place within the surrounding graph.
Correct?

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:316: UnifyReturn(&jsgraph);
On a high level it is terribly confusing when "jsgraph" and "graph" refers to
the outer or the inner graph. I think this should be cleaned up a bit. One
suggestion would be to have a class around the inner graph (e.g. InnerGraph,
SubGraph, Inlinee, ...) that has helper methods to perform mutations on the
inlinee. Having all of the inlining logic in static top-level methods smells
dangerous.

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

https://codereview.chromium.org/453833003/diff/20001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:163: // Specialize the code to the context as
aggressively as possible.
nit: Comment seems off, lets either drop it or fix it.

https://codereview.chromium.org/453833003/diff/20001/test/cctest/compiler/fun...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/453833003/diff/20001/test/cctest/compiler/fun...
test/cctest/compiler/function-tester.h:27: static void AssertStackDepth(const
v8::FunctionCallbackInfo<v8::Value>& args) {
All of this machinery is pretty specific to inlining and how
test-run-inlining.cc performs it's checking. Also it will change once the
stack-walker is actually fixed. This makes me think that we should move this
into test-run-inlining.cc instead.

We could just have a static InstallHelperFunction() there that takes the
existing global object and installs this function on it. Note that the
FunctionTemplate can be instantiated (i.e. FunctionTemplate::GetFunction) to get
an actual function object that can then be easily set on the existing global
object. Look at e.g. CallAPIFunctionOnNonObject in test-api.cc how this can be
done.

[sigurds, 2014-08-14 09:54:43]

PTAL

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

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:75: /// Ensure that only a single return reaches the
end node.
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> nit: Two slashes ought to be enough for everyone. :)

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:121: void moveWithDependencies(JSGraph* graph, Node*
node, Node* old_block,
On 2014/08/12 20:04:45, Michael Starzinger wrote:
> style: Uppercase "M". Also needs to be static.

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:155: static void moveAllControlNodes(Node* from,
Node* to) {
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> style: Uppercase "M".

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:166: void DoInline(Zone* zone, Node* call,
CompilationInfo* info, JSGraph* jsgraph) {
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> Either make this a (private) member method of JSInliner instead of passing
> everything around as arguments or at least make this static.

Made it a private method. (Saves Zone* argument)

info and jsgraph belong to the inlinee, so I have to pass it in.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:237: if (IsStackGuard(stackGuard)) {
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> Let's just drop the removal of the stack-guard for now. This will break once
we
> implement actual stack-guards (instead of runtime calls) and should not affect
> correctness.

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:268: SmartArrayPointer<char> name =
function->shared()->DebugName()->ToCString();
On 2014/08/12 20:04:45, Michael Starzinger wrote:
> nit: The "name" is only use for tracing, please move into the tracing blocks
> (even though it's duplicated).

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:272: printf("Not Inlining %s into %s because inlinee
is native\n", name.get(),
On 2014/08/12 20:04:45, Michael Starzinger wrote:
> nit: s/printf/PrintF/

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:291: printf(
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> nit: s/printf/PrintF/

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:304: printf("Inlining %s into %s\n", name.get(),
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> nit: s/printf/PrintF/

Done.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:309:
graph.SetNextNodeId(jsgraph_->graph()->NodeCount());
On 2014/08/12 20:04:45, Michael Starzinger wrote:
> I assume this is only temporary for now that we don't copy the nodes from the
> inlinee but rather create the nodes in place within the surrounding graph.
> Correct?
Yes. To copying the graph will follow as a separate cl, and then SetNextNodeId
can be removed again.

https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:316: UnifyReturn(&jsgraph);
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> On a high level it is terribly confusing when "jsgraph" and "graph" refers to
> the outer or the inner graph. I think this should be cleaned up a bit. One
> suggestion would be to have a class around the inner graph (e.g. InnerGraph,
> SubGraph, Inlinee, ...) that has helper methods to perform mutations on the
> inlinee. Having all of the inlining logic in static top-level methods smells
> dangerous.

I agree. Effectively, we are reimplementing the pipeline here, which we should
not. I'd like to integrate this code better with the pipeline, but not in this
CL.

I make it more clear for now who is the inlinee, I introduced the class Inlinee,
which also makes InlineAtCall more readable.

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

https://codereview.chromium.org/453833003/diff/20001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:163: // Specialize the code to the context as
aggressively as possible.
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> nit: Comment seems off, lets either drop it or fix it.

Done.

https://codereview.chromium.org/453833003/diff/20001/test/cctest/compiler/fun...
File test/cctest/compiler/function-tester.h (right):

https://codereview.chromium.org/453833003/diff/20001/test/cctest/compiler/fun...
test/cctest/compiler/function-tester.h:27: static void AssertStackDepth(const
v8::FunctionCallbackInfo<v8::Value>& args) {
On 2014/08/12 20:04:46, Michael Starzinger wrote:
> All of this machinery is pretty specific to inlining and how
> test-run-inlining.cc performs it's checking. Also it will change once the
> stack-walker is actually fixed. This makes me think that we should move this
> into test-run-inlining.cc instead.
> 
> We could just have a static InstallHelperFunction() there that takes the
> existing global object and installs this function on it. Note that the
> FunctionTemplate can be instantiated (i.e. FunctionTemplate::GetFunction) to
get
> an actual function object that can then be easily set on the existing global
> object. Look at e.g. CallAPIFunctionOnNonObject in test-api.cc how this can be
> done.

Done.

[titzer, 2014-08-14 10:24:31]

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

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:27: static void ReplaceEffectfulWithValue(Node*
node, Node* value) {
Can you find a place for this routine? I think this is the third copy now. Also,
you updated the one in context specialization to remove all inputs but not this
one.

[Michael Starzinger, 2014-08-14 10:36:23]

LGTM with one final round of comments to address.

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

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:48: explicit InlinerVisitor(JSInliner* spec) :
spec_(spec) {}
nit: s/spec/inliner/

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:62: JSInliner* spec_;
nit: s/spec_/inliner_/

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining.h
File src/compiler/js-inlining.h (right):

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.h:10: #include "src/contexts.h"
nit: Both the graph-reducer.h and the contexts.h include should not be needed I
think.

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

https://codereview.chromium.org/453833003/diff/40001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:199: SourcePositionTable::Scope pos_(&source_positions,
nit: Let's drop the trialing underscore from "pos" here and a few lines above in
line 190 as well.

https://codereview.chromium.org/453833003/diff/40001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/453833003/diff/40001/src/flag-definitions.h#n...
src/flag-definitions.h:341: DEFINE_STRING(turbo_inlining_filter, "*",
As discussed offline: Let's drop the filter flag again, since it is not (yet)
needed and also it is intuitively unclear whether the filter should apply to the
surrounding function or the inlinee.

[sigurds, 2014-08-19 11:31:19]

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

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:27: static void ReplaceEffectfulWithValue(Node*
node, Node* value) {
On 2014/08/14 10:24:31, titzer wrote:
> Can you find a place for this routine? I think this is the third copy now.
Also,
> you updated the one in context specialization to remove all inputs but not
this
> one.

Done.

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:48: explicit InlinerVisitor(JSInliner* spec) :
spec_(spec) {}
On 2014/08/14 10:36:22, Michael Starzinger wrote:
> nit: s/spec/inliner/

Done.

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.cc:62: JSInliner* spec_;
On 2014/08/14 10:36:22, Michael Starzinger wrote:
> nit: s/spec_/inliner_/

Done.

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining.h
File src/compiler/js-inlining.h (right):

https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
src/compiler/js-inlining.h:10: #include "src/contexts.h"
On 2014/08/14 10:36:22, Michael Starzinger wrote:
> nit: Both the graph-reducer.h and the contexts.h include should not be needed
I
> think.

Done.

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

https://codereview.chromium.org/453833003/diff/40001/src/compiler/pipeline.cc...
src/compiler/pipeline.cc:199: SourcePositionTable::Scope pos_(&source_positions,
On 2014/08/14 10:36:22, Michael Starzinger wrote:
> nit: Let's drop the trialing underscore from "pos" here and a few lines above
in
> line 190 as well.

Done.

https://codereview.chromium.org/453833003/diff/40001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/453833003/diff/40001/src/flag-definitions.h#n...
src/flag-definitions.h:341: DEFINE_STRING(turbo_inlining_filter, "*",
On 2014/08/14 10:36:22, Michael Starzinger wrote:
> As discussed offline: Let's drop the filter flag again, since it is not (yet)
> needed and also it is intuitively unclear whether the filter should apply to
the
> surrounding function or the inlinee.

Done.

[titzer, 2014-08-19 12:04:16]

On 2014/08/19 11:31:19, sigurds wrote:
>
https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining.cc
> File src/compiler/js-inlining.cc (right):
> 
>
https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
> src/compiler/js-inlining.cc:27: static void ReplaceEffectfulWithValue(Node*
> node, Node* value) {
> On 2014/08/14 10:24:31, titzer wrote:
> > Can you find a place for this routine? I think this is the third copy now.
> Also,
> > you updated the one in context specialization to remove all inputs but not
> this
> > one.
> 
> Done.
> 
>
https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
> src/compiler/js-inlining.cc:48: explicit InlinerVisitor(JSInliner* spec) :
> spec_(spec) {}
> On 2014/08/14 10:36:22, Michael Starzinger wrote:
> > nit: s/spec/inliner/
> 
> Done.
> 
>
https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
> src/compiler/js-inlining.cc:62: JSInliner* spec_;
> On 2014/08/14 10:36:22, Michael Starzinger wrote:
> > nit: s/spec_/inliner_/
> 
> Done.
> 
>
https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining.h
> File src/compiler/js-inlining.h (right):
> 
>
https://codereview.chromium.org/453833003/diff/40001/src/compiler/js-inlining...
> src/compiler/js-inlining.h:10: #include "src/contexts.h"
> On 2014/08/14 10:36:22, Michael Starzinger wrote:
> > nit: Both the graph-reducer.h and the contexts.h include should not be
needed
> I
> > think.
> 
> Done.
> 
> https://codereview.chromium.org/453833003/diff/40001/src/compiler/pipeline.cc
> File src/compiler/pipeline.cc (right):
> 
>
https://codereview.chromium.org/453833003/diff/40001/src/compiler/pipeline.cc...
> src/compiler/pipeline.cc:199: SourcePositionTable::Scope
pos_(&source_positions,
> On 2014/08/14 10:36:22, Michael Starzinger wrote:
> > nit: Let's drop the trialing underscore from "pos" here and a few lines
above
> in
> > line 190 as well.
> 
> Done.
> 
> https://codereview.chromium.org/453833003/diff/40001/src/flag-definitions.h
> File src/flag-definitions.h (right):
> 
>
https://codereview.chromium.org/453833003/diff/40001/src/flag-definitions.h#n...
> src/flag-definitions.h:341: DEFINE_STRING(turbo_inlining_filter, "*",
> On 2014/08/14 10:36:22, Michael Starzinger wrote:
> > As discussed offline: Let's drop the filter flag again, since it is not
(yet)
> > needed and also it is intuitively unclear whether the filter should apply to
> the
> > surrounding function or the inlinee.
> 
> Done.

LGTM

[sigurds, 2014-08-19 12:23:32]

Committed patchset #4 manually as 23197 (presubmit successful).