Thanks a lot for comments, Kevin.
As discussed on IM, I'm postponing hoisting of serialized scope info into
parser
itself for this CL.
Would appreciated the next round.
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen-instructions.cc#newcode1230
src/hydrogen-instructions.cc:1230: stream->Add("[%d->%d]]",
context_chain_length(), slot_index());
On 2010/12/20 12:05:44, Kevin Millikin wrote:
I think you have an extra "]" here. I'm not wild about the ASCII
arrow, because
it doesn't really signify anything. "(%d,%d) seems fine to me.
Done.
I used -> syntax to mimic C/C++ structure's field access
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc
File src/hydrogen.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2934
src/hydrogen.cc:2934: variable->AsSlot()->type() == Slot::CONTEXT) {
On 2010/12/20 12:05:44, Kevin Millikin wrote:
This predicate (->AsSlot() != NULL && ->AsSlot()->type ==
Slot::CONTEXT) should
probably be defined on the Variable class as something like
"IsInContext()" or
"IsHeapAllocated()".
Done.
I named IsContextSlot as IMHO IsHeapAllocated rather leaks
implementation detail and IsInContext looks somewhat vague for me. But
if you prefer any other name, just name it :)
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode2965
src/hydrogen.cc:2965: BAILOUT("reference to
non-stack-allocated/non-global variable");
On 2010/12/20 12:05:44, Kevin Millikin wrote:
You should probably change this bailout string. All that's left is
Slot::LOOKUP, right?
Done.
Please check if wording is fine.
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3463
src/hydrogen.cc:3463: if (var->AsSlot() != NULL && var->AsSlot()->type()
== Slot::CONTEXT) {
On 2010/12/20 12:05:44, Kevin Millikin wrote:
Instead of an early bailout, it would be better if this code should be
structured like the code in VisitVariableProxy, explicitly handling
the cases:
if (/* global */) ...
else if (/* stack-allocated */) ...
else /* the rest, bailout */
Done.
http://codereview.chromium.org/5753005/diff/27001/src/hydrogen.cc#newcode3472
src/hydrogen.cc:3472: } else {
On 2010/12/20 12:05:44, Kevin Millikin wrote:
Because here we don't want to have Slot::LOOKUP fall through the
cracks.
Done.
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia32.cc#newcode1823
src/ia32/lithium-codegen-ia32.cc:1823: HLoadContextSlot* hinstr =
instr->hydrogen();
On 2010/12/20 12:05:44, Kevin Millikin wrote:
It's typical for the Lithium instructions to have accessors like
slot_index and
context_chain_length that just forward to the hydrogen instruction.
Done.
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-codegen-ia32.cc#newcode1824
src/ia32/lithium-codegen-ia32.cc:1824: Comment(";;; Load context slot %d
at %d",
On 2010/12/20 12:05:44, Kevin Millikin wrote:
There will automatically be a comment with the printed representation
of the
instruction without having to emit it explicitly.
You should just have to override the print data function for
LLoadContextSlot to
print the height and offset.
Done.
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/ia32/lithium-ia32.cc#newcode274
src/ia32/lithium-ia32.cc:274: hydrogen()->PrintDataTo(stream);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
I wouldn't forward to the hydrogen instruction like this. There is no
guarantee
that the printed representation of a hydrogen instruction makes sense
in a
Lithium context.
Done.
http://codereview.chromium.org/5753005/diff/27001/src/objects.cc
File src/objects.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/objects.cc#newcode5381
src/objects.cc:5381: || code() == shared()->code());
On 2010/12/20 12:05:44, Kevin Millikin wrote:
For some reason we like to put || on the first line. In any case, the
second
line should be lined up under the "s" in shared() above it.
Done.
http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc
File src/runtime-profiler.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#newcode139
src/runtime-profiler.cc:139: && function->shared()->code() == code;
On 2010/12/20 12:05:44, Kevin Millikin wrote:
Why is this last check necessary?
Oops, remnants of previous attempts. Removed.
http://codereview.chromium.org/5753005/diff/27001/src/runtime-profiler.cc#newcode165
src/runtime-profiler.cc:165: ||
!function->shared()->allows_lazy_compilation()) {
On 2010/12/20 12:05:44, Kevin Millikin wrote:
It makes more sense to move this check down below with the other
checks on the
shared code (after SharedFunctionInfo* shared = function->shared()')
Done.
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc
File src/scopes.cc (right):
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode133
src/scopes.cc:133: // At some point we might want to provide outer
scopes to
On 2010/12/20 12:05:44, Kevin Millikin wrote:
Keep this comment near the ASSERT it mentions.
Done.
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode241
src/scopes.cc:241: return new Variable(this, name, Variable::VAR, true,
Variable::ARGUMENTS);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
We don't want to return a fresh variable every time we lookup the same
symbol in
the same scope. We lose the property that the same identifier always
resolves
to the same variable in the same scope.
Thanks a lot for spotting this.
Thinking more about it: resolved scopes should be never innermost scopes
and thus apparently they never need to resolve arguments name as this
way or enough it must be always looked up by the innermost scope,
correct? Tests pass.
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode246
src/scopes.cc:246: int index = scope_info_->ContextSlotIndex(*name,
&mode);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
We also want to assert (at least) that we do not find this name as a
stack-allocated variable in this scope. If we do then something has
gone wrong.
Done.
http://codereview.chromium.org/5753005/diff/27001/src/scopes.cc#newcode246
src/scopes.cc:246: int index = scope_info_->ContextSlotIndex(*name,
&mode);
On 2010/12/20 12:05:44, Kevin Millikin wrote:
We also want to assert (at least) that we do not find this name as a
stack-allocated variable in this scope. If we do then something has
gone wrong.
Done.
http://codereview.chromium.org/5753005/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
