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.
nit: Two slashes ought to be enough for everyone. :)
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,
style: Uppercase "M". Also needs to be static.
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) {
style: Uppercase "M".
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) {
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.cc#newcode237
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.cc#newcode268
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.cc#newcode272
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.cc#newcode291
src/compiler/js-inlining.cc:291: printf(
nit: s/printf/PrintF/
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(),
nit: s/printf/PrintF/
https://codereview.chromium.org/453833003/diff/20001/src/compiler/js-inlining.cc#newcode309
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.cc#newcode316
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#newcode163
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/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) {
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.
https://codereview.chromium.org/453833003/
--
--
v8-dev mailing list
[email protected]
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 [email protected].
For more options, visit https://groups.google.com/d/optout.
