LGTM
https://chromiumcodereview.appspot.com/10031031/diff/1001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://chromiumcodereview.appspot.com/10031031/diff/1001/src/hydrogen-instructions.h#newcode523 src/hydrogen-instructions.h:523: #undef COUNT_FLAG I think it would be nice to provide functions: GVNFlag ChangesFlagFromInt(int x) { return x * 2; } GVNFlag DependsOnFlagFromInt(int x) { return x * 2 + 1; } and then use them instead of just multiplying by 2, which is fairly opaque. https://chromiumcodereview.appspot.com/10031031/diff/1001/src/hydrogen-instructions.h#newcode804 src/hydrogen-instructions.h:804: result.Remove(kChangesNewSpace); The name makes it sound like any write to new space is tracked, but actually it is just tracking whether anything might happen to promote objects from new space. Would kChangesNewSpacePromotion be better? https://chromiumcodereview.appspot.com/10031031/diff/1001/src/hydrogen-instructions.h#newcode4053 src/hydrogen-instructions.h:4053: offset_(offset) { Should you not initialize new_space_dominator_ here? https://chromiumcodereview.appspot.com/10031031/diff/1001/src/hydrogen.cc File src/hydrogen.cc (right): https://chromiumcodereview.appspot.com/10031031/diff/1001/src/hydrogen.cc#newcode1700 src/hydrogen.cc:1700: HSideEffectMap& dominators) { This should be a pointer argument: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Arguments https://chromiumcodereview.appspot.com/10031031/diff/1001/src/hydrogen.cc#newcode1741 src/hydrogen.cc:1741: GVNFlag depends_on_flag = static_cast<GVNFlag>(i * 2 + 1); This would be cleaner with a variable to hold dominators[i] that you can use in the 4 places below. https://chromiumcodereview.appspot.com/10031031/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
