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.cc#newcode75
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.cc#newcode121
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.cc#newcode155
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.cc#newcode166
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.cc#newcode237
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.cc#newcode268
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.cc#newcode272
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.cc#newcode291
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.cc#newcode304
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.cc#newcode309
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.cc#newcode316
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#newcode163
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/function-tester.h
File test/cctest/compiler/function-tester.h (right):
https://codereview.chromium.org/453833003/diff/20001/test/cctest/compiler/function-tester.h#newcode27
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.
https://codereview.chromium.org/453833003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
