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); 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 { 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. 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 s/sucsessors/successors/g 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) { 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) { Maybe it would make sense to introduce a HPredecessorIterator, similar to HSuccessorIterator for more concise code. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode358 src/hydrogen.cc:358: MarkAsLoopSuccessorDominator(); I guess this should be: candidate_dominator->MarkAsLoopSuccessorDominator(); otherwise no other block will be marked. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode362 src/hydrogen.cc:362: for (int current = 0; current < successor_count; ++current) { You can use HSuccessorIterator here. 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) { Comment on why resetting the loop side effect sets. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode1552 src/hydrogen.cc:1552: !instr->HasObservableSideEffects() && maybe just say: if (!can_hoist && instr->IsTransitionElementsKind()) I think this should be generalized as necessary in the future. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode1572 src/hydrogen.cc:1572: can_hoist = !not_in_nested_loop && The double negation !not... should be avoided for readability. http://codereview.chromium.org/9141016/diff/20017/src/hydrogen.cc#newcode1597 src/hydrogen.cc:1597: // SideEffects flags to not prevent hosting OneTimeSideEffecct 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. 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() { Please add output to --trace-hydrogen so that you can identify the marked blocks in the visualizer. http://codereview.chromium.org/9141016/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
