LGTM with minor comments.
Also, I think it would be nice to add a few sentences of high-level comments
giving an overview of how the code works (does a DFS through the dominator
tree,
keeps a stack with status information about currently active levels in the
zone).
https://chromiumcodereview.appspot.com/10520004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):
https://chromiumcodereview.appspot.com/10520004/diff/1/src/hydrogen.cc#newcode1362
src/hydrogen.cc:1362: *this = *other; // Calls operator=
nit: full stop at the end of the comment please.
https://chromiumcodereview.appspot.com/10520004/diff/1/src/hydrogen.cc#newcode1858
src/hydrogen.cc:1858: *dominator = NULL;
IIUC, the value of *dominator doesn't matter when you return NULL, since
that indicates the end of the iteration.
https://chromiumcodereview.appspot.com/10520004/diff/1/src/hydrogen.cc#newcode1859
src/hydrogen.cc:1859: result = NULL;
Unnecessary; result == NULL here anyway.
https://chromiumcodereview.appspot.com/10520004/diff/1/src/hydrogen.cc#newcode2009
src/hydrogen.cc:2009: HBasicBlock *dominator_block;
nit: s/ */* /
https://chromiumcodereview.appspot.com/10520004/diff/1/src/hydrogen.cc#newcode2015
src/hydrogen.cc:2015: // No need to copy the map for the last child in
the dominator tree.
I think this comment should go inside
GvnBasicBlockState::next_dominated() now.
https://chromiumcodereview.appspot.com/10520004/diff/1/src/hydrogen.cc#newcode2028
src/hydrogen.cc:2028:
CollectSideEffectsOnPathsToDominatedBlock(dominator_block, dominated);
nit: long line
https://chromiumcodereview.appspot.com/10520004/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
