Addressed those comments that only required minor changes; more refactorings
will come.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc
File src/hydrogen.cc (right):
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode823
src/hydrogen.cc:823: // branch. However, we must pretend that the "then"
branch is reachable.
On 2013/05/23 12:34:55, titzer wrote:
Why must we pretend that it's reachable?
So that the graph builder visits it and any environment lookups inside
it. I've updated the comment.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode2541
src/hydrogen.cc:2541: if (pass == kIterateUntilFixedPoint &&
!worklist->Contains(block_id)) {
On 2013/05/23 12:34:55, titzer wrote:
Your loop goes through the block order backwards. No problem. Any
order would
work. Luckily the block order has been computed carefully in the
previous phases
to match reverse-post order, thus making this loop efficient.
Nevertheless your
main loop loops over all the blocks each time. The first time it will
visit each
block, but then subsequent iterations (because of loops) will be
sparse.
I am not sure if now is the right time for a helper iterator, but we
do seem to
be iterating manually in various order all over the place in the code.
If we
have other backwards dataflow problems, we might consider either
repurposing or
generalizing the PostorderProcessor to help here.
I'm explicitly going backwards in order to reduce the number of needed
iterations.
I'd prefer not to add any unrelated refactorings to this CL (and,
frankly, I'm not convinced we need a generic helper iterator. Ordering
blocks once should be quite sufficient.)
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode2568
src/hydrogen.cc:2568: // a simulate before an EnterInlined.
On 2013/05/23 12:34:55, titzer wrote:
Scary and brittle, and no ASSERT.
ASSERT added.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode5051
src/hydrogen.cc:5051: EnvironmentLivenessAnalysis();
On 2013/05/23 12:34:55, titzer wrote:
This phase should be guarded by a flag, and should be named verbly (as
other
phases) to reflect that it makes modifications to the graph.
Done.
Side note: "DeadCodeElimination" isn't a verb either ;-)
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.h
File src/hydrogen.h (right):
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.h#newcode158
src/hydrogen.h:158: void MarkAsInlineReturnTarget(HBasicBlock*
enter_inlined_block) {
On 2013/05/23 12:34:55, titzer wrote:
This name is somewhat difficult to parse. Is this the first block of
the inlined
method? If so, inlined_entry_block might be a better name.
Done. (It's the block containing the HEnterInlined. We don't start a new
block when inlining.)
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.h#newcode372
src/hydrogen.h:372: void update_biggest_environment_ever(int
environment_size) {
On 2013/05/23 12:34:55, titzer wrote:
Cute name, but "maximum_environment_size" would also work.
Done.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.h#newcode426
src/hydrogen.h:426: void ZapEnvironmentSlot(int index, HSimulate*
simulate);
On 2013/05/23 12:34:55, titzer wrote:
Maybe "Kill" or "Null" or "Remove"?
We use "zap" in other places, so I'd like to keep the name.
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.
