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.
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.
