On 2013/06/03 17:50:38, Jakob wrote:
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc
File src/hydrogen-environment-liveness.cc (right):
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc#newcode84
src/hydrogen-environment-liveness.cc:84: HBasicBlock* block) {
On 2013/05/29 16:58:05, titzer wrote:
> E.g. here, it would be more clear to pass in the temporary live_
bitvector,
so
> that it's clear the caller expects the method to modify it.
Done.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc#newcode114
src/hydrogen-environment-liveness.cc:114: if (collect_markers_) {
On 2013/05/29 16:58:05, titzer wrote:
> This is a little tricky; maybe a comment that we only collect markers
the
first
> time through the graph.
Done.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc#newcode149
src/hydrogen-environment-liveness.cc:149: case HValue::kDeoptimize: {
On 2013/05/29 16:58:05, titzer wrote:
> What about kSoftDeoptimize?
Usage is different.
HSoftDeoptimize instructions do not affect live ranges in any way, so they
don't
need any special handling here.
There are uses of HDeoptimize however that assume that the entire
environment
is
available. I'm hoping that this will change eventually, but that's a
different
issue.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc#newcode168
src/hydrogen-environment-liveness.cc:168: // Values in the environment are
kept
alive by every subsequent LInstruction
On 2013/05/29 16:58:05, titzer wrote:
> Some reordering of the sentences might make this paragraph clearer. E.g.
first
> sentence should probably be something to the effect of "Trim live
ranges of
> environment slots by doing explicit liveness analysis". And then why
this is
> necessary (stuff is live too long because of environments, and then
maybe
any
> relevant details.
Done, and moved to the class, where this comment really makes more sense
now.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc#newcode205
src/hydrogen-environment-liveness.cc:205: // Reached the start of the
block,
do
necessary bookkeeping.
On 2013/05/29 16:58:05, titzer wrote:
> Explain bookkeeping steps. E.g. "propagate liveness to predecessors"
would
do.
Done.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h
File src/hydrogen-environment-liveness.h (right):
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h#newcode37
src/hydrogen-environment-liveness.h:37:
On 2013/05/29 16:58:05, titzer wrote:
> Documentation. At least a sentence. Perhaps
EnvironmentSlotLivenessAnalyzer
> instead of *Analysis?
Done.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h#newcode40
src/hydrogen-environment-liveness.h:40: explicit
EnvironmentSlotLivenessAnalysis(HGraph* graph)
On 2013/05/29 16:58:05, titzer wrote:
> Please put this constructor in the .cc file, if C++ will allow you.
Done. Good point, dunno why it didn't occur to me before.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h#newcode83
src/hydrogen-environment-liveness.h:83: ZoneScope zone_scope_;
On 2013/05/29 16:58:05, titzer wrote:
> Would like to see some comments on these fields, e.g. that the various
lists
are
> indexed by blockid.
Done.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h#newcode92
src/hydrogen-environment-liveness.h:92: BitVector* live_;
On 2013/05/29 16:58:05, titzer wrote:
> I think it's best to turn this field into a local and pass it between
the
> various methods; following the effects in the Update* methods will be
easier.
Done.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen.cc#newcode829
src/hydrogen.cc:829: // constructs within it.
On 2013/05/29 16:58:05, titzer wrote:
> I'm still perplexed why we need to insert a dead branch.
I'm not sure what information is missing from the comment. Previously, the
graph
builder would skip over unreachable branches, where unreachability may be
based
solely on type feedback and guarded by corresponding checks. If there's an
environment lookup happening in that branch that keeps a value alive, not
visiting it means not noticing the lookup, not keeping the value alive,
and
materializing "undefined" instead of the proper value when we deopt due
to the
branch becoming reachable after all. So we must make sure that all live
ranges
can be determined accurately from the graph. (We have test coverage for
this.)
https://codereview.chromium.org/15533004/diff/29001/test/mjsunit/debug-evaluate-locals-optimized-double.js
File test/mjsunit/debug-evaluate-locals-optimized-double.js (right):
https://codereview.chromium.org/15533004/diff/29001/test/mjsunit/debug-evaluate-locals-optimized-double.js#newcode187
test/mjsunit/debug-evaluate-locals-optimized-double.js:187: debugger; //
Breakpoint.
On 2013/05/29 16:58:05, titzer wrote:
> Not sure why we are changing these tests. Does something break with
debugging?
We need to extend the live ranges of the values that are being inspected
beyond
the point where they are inspected. The "debugger;" statement triggers
the big
"listener()" function above (lines 65ff.), which has expectations for the
local
variables in every stack frame.
This does mean that users inspecting stack frames where local values have
been
zapped at the end of their live range will see "undefined" unexpectedly.
Unfortunately, there's nothing we can do to prevent that, because there's
no
way
to predict if/when a stack frame will be inspected.
LGTM. I am certain people are going to ask why their local variables aren't
available in the debugger after they die. Prepare to point them to your
flag :)
https://codereview.chromium.org/15533004/
--
--
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/groups/opt_out.
