Almost there, just mostly documentation.
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) {
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.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc#newcode114
src/hydrogen-environment-liveness.cc:114: if (collect_markers_) {
This is a little tricky; maybe a comment that we only collect markers
the first time through the graph.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.cc#newcode149
src/hydrogen-environment-liveness.cc:149: case HValue::kDeoptimize: {
What about kSoftDeoptimize?
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
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.
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.
Explain bookkeeping steps. E.g. "propagate liveness to predecessors"
would do.
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:
Documentation. At least a sentence. Perhaps
EnvironmentSlotLivenessAnalyzer instead of *Analysis?
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h#newcode40
src/hydrogen-environment-liveness.h:40: explicit
EnvironmentSlotLivenessAnalysis(HGraph* graph)
Please put this constructor in the .cc file, if C++ will allow you.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h#newcode83
src/hydrogen-environment-liveness.h:83: ZoneScope zone_scope_;
Would like to see some comments on these fields, e.g. that the various
lists are indexed by blockid.
https://codereview.chromium.org/15533004/diff/29001/src/hydrogen-environment-liveness.h#newcode92
src/hydrogen-environment-liveness.h:92: BitVector* live_;
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.
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.
I'm still perplexed why we need to insert a dead branch.
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.
Not sure why we are changing these tests. Does something break with
debugging?
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.
