Feedback addressed, please take another look.
http://codereview.chromium.org/9141016/diff/20017/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/9141016/diff/20017/src/hydrogen-instructions.h#newcode4170 src/hydrogen-instructions.h:4170: SetGVNFlag(kDependsOnMaps); But if we ever add one, then it will be incorrect to hoist the transitions about it, so I'd prefer to keep this in. On 2012/02/02 14:59:50, fschneider wrote:
There is currently no instruction that has kChangesMaps set (except
calls that
have all side effects).
http://codereview.chromium.org/9141016/diff/20017/src/hydrogen-instructions.h#newcode4188 src/hydrogen-instructions.h:4188: virtual bool HasOneTimeSideEffects() const { On 2012/02/02 14:59:50, fschneider wrote:
It may be not worth making a predicate that applies to exactly one
instruction a
separate virtual function (unless we plan to get more instructions
like this).
I'd just use IsTransitionElementsKind() for now.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode330 src/hydrogen.cc:330: int outstanding_sucsessors = 1; // one edge from the pre-header On 2012/02/02 14:59:50, fschneider wrote:
s/sucsessors/successors/g
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode333 src/hydrogen.cc:333: for (int j = block_id(); j <= last->block_id(); ++j) { You have to iterate over every block's successors, even the header, so the header must be included in the iteration over the loop. On 2012/02/02 14:59:50, fschneider wrote:
Maybe this be simplified: The header is always marked, so there is no
need to
include it in the iteration:
Would it work to initialize outstanding_successors to 0 and start at
block_id()
+ 1 instead?
Then you could also save detecting back edges below.
http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode338 src/hydrogen.cc:338: for (int i = 0; i < predecessor_count; ++i) { On 2012/02/02 14:59:50, fschneider wrote:
Maybe it would make sense to introduce a HPredecessorIterator, similar
to
HSuccessorIterator for more concise code.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode358 src/hydrogen.cc:358: MarkAsLoopSuccessorDominator(); On 2012/02/02 14:59:50, fschneider wrote:
I guess this should be:
candidate_dominator->MarkAsLoopSuccessorDominator();
otherwise no other block will be marked.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode362 src/hydrogen.cc:362: for (int current = 0; current < successor_count; ++current) { On 2012/02/02 14:59:50, fschneider wrote:
You can use HSuccessorIterator here.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode1478 src/hydrogen.cc:1478: for (int i = 0; i < loop_side_effects_.length(); ++i) { On 2012/02/02 14:59:50, fschneider wrote:
Comment on why resetting the loop side effect sets.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode1552 src/hydrogen.cc:1552: !instr->HasObservableSideEffects() && On 2012/02/02 14:59:50, fschneider wrote:
maybe just say:
if (!can_hoist && instr->IsTransitionElementsKind())
I think this should be generalized as necessary in the future.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode1572 src/hydrogen.cc:1572: can_hoist = !not_in_nested_loop && On 2012/02/02 14:59:50, fschneider wrote:
The double negation !not... should be avoided for readability.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode1597 src/hydrogen.cc:1597: // SideEffects flags to not prevent hosting OneTimeSideEffecct On 2012/02/02 14:59:50, fschneider wrote:
I find the double negation in the comment confusing. How about the
following:
If an instruction is not hoisted, we have to account for its side
effects when
hoisting later HTransitionElementsKind instructions.
Done. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.h File src/hydrogen.h (right): http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.h#newcode156 src/hydrogen.h:156: void MarkAsLoopSuccessorDominator() { On 2012/02/02 14:59:50, fschneider wrote:
Please add output to --trace-hydrogen so that you can identify the
marked blocks
in the visualizer.
Done. http://codereview.chromium.org/9141016/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
