Thanks for the second round of comments!
https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-operator.h
File src/compiler/common-operator.h (right):
https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-operator.h#newcode35
src/compiler/common-operator.h:35: enum FrameStateType { JS_FRAME,
ARGUMENTS_ADAPTOR };
On 2014/09/16 14:12:56, Michael Starzinger wrote:
nit: This needs some comments I think. How about the following?
// The type of stack frame that a FrameState node represents.
enum FrameStateType {
JS_FRAME, // Represents an unoptimized JavaScriptFrame.
ARGUMENTS_ADAPTOR // Represents an ArgumentsAdaptorFrame.
};
Done.
https://codereview.chromium.org/573703002/diff/40001/src/compiler/common-operator.h#newcode57
src/compiler/common-operator.h:57: Unique<JSFunction> jsfunction_;
On 2014/09/16 14:12:56, Michael Starzinger wrote:
nit: IMHO we should just use Handle<JSFunction> instead of
Unique<JSFunction>
here. Not sure what a Unique buys us here.
For posterity only: Also this will not necessarily be a constant
JSFunction in
the long run. At some point we want to support inlining of
non-constant
functions when we know they have a constant underlying
SharedFunctionInfo. At
that point this will become an input operand instead of a constant
parameter.
Correct?
Yes, absolutely correct. I propose leaving it as-is, and fixing it when
we support inlining based on SharedFunctionInfo without constant
closure.
https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining.cc
File src/compiler/js-inlining.cc (right):
https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining.cc#newcode311
src/compiler/js-inlining.cc:311: class JSCallFunctionAccessor {
On 2014/09/16 14:12:56, Michael Starzinger wrote:
While such a thing would be nice to have, spreading it all over the
place is a
slippery slop. In an ideal world such an accessor might even be
generated from a
DSL (I know I am dreaming here). I could be convinced to let this pass
if we add
a big fat TODO(turbofan) here that this should be further nailed down
and moved
into the right place later.
I absolutely agree. However, without this thing the code that uses it
now becomes cryptic. I am strongly in favor of the big TODO.
https://codereview.chromium.org/573703002/diff/40001/src/compiler/js-inlining.cc#newcode398
src/compiler/js-inlining.cc:398: info.EnableDeoptimizationSupport();
On 2014/09/16 14:12:56, Michael Starzinger wrote:
OMG, we really should try to find a way to do this lazily when we
actually
deopt, instead of eagerly throughout the compilation pipeline. Could
we also
leave a TODO(turbofan) comment in that regard?
Done.
https://codereview.chromium.org/573703002/
--
--
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.
