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.
Why must we pretend that it's reachable?
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode2539
src/hydrogen.cc:2539: while (pass != kDone) {
I realize there are some tricky cases inside this loop, but this enum /
pass flag that completely changes the loop behavior is way too
complication. Please extract this to two or three loops and
loop-unswitch this code.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode2541
src/hydrogen.cc:2541: if (pass == kIterateUntilFixedPoint &&
!worklist->Contains(block_id)) {
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.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode2568
src/hydrogen.cc:2568: // a simulate before an EnterInlined.
Scary and brittle, and no ASSERT.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode2613
src/hydrogen.cc:2613: } else if (instr->IsEnterInlined()) {
There is considerably more going on here than meets the eye; you are
doing liveness analysis on different environments for different inlined
methods. This currently works because you are only using the environment
bind and environment lookup as essentially reads and writes and
thankfully before any other optimizations happen, code from different
inlined methods is not mixed up into the same basic blocks. I am not
sure if this would hold if the order of this phase were changed. Also,
below you are making assumptions about what order instructions will
appear in inlined methods. This makes this phase brittle and restricts
changes in other phases. Please consider ways to relax these
restrictions, or do something conservative when they do not hold.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.cc#newcode5051
src/hydrogen.cc:5051: EnvironmentLivenessAnalysis();
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.
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) {
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.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.h#newcode372
src/hydrogen.h:372: void update_biggest_environment_ever(int
environment_size) {
Cute name, but "maximum_environment_size" would also work.
https://codereview.chromium.org/15533004/diff/1008/src/hydrogen.h#newcode426
src/hydrogen.h:426: void ZapEnvironmentSlot(int index, HSimulate*
simulate);
Maybe "Kill" or "Null" or "Remove"?
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.
