LGTM with comments.
https://chromiumcodereview.appspot.com/11783055/diff/2002/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://chromiumcodereview.appspot.com/11783055/diff/2002/src/hydrogen-instructions.h#newcode351 src/hydrogen-instructions.h:351: Representation KeyedAccessIndexRequirement() { nit: I'm a bit unhappy about adding instruction-specific logic to the Representation class. Three alternative suggestions, pick what you like: 1) Inline it. It's only one line, after all. 2) Move to a static method of ArrayInstructionInterface. 3) Move to a file-global static function "Representation ArrayIndexRepresentation(Representation r) {...}" as close as possible to the two call sites. https://chromiumcodereview.appspot.com/11783055/diff/2002/src/hydrogen.cc File src/hydrogen.cc (right): https://chromiumcodereview.appspot.com/11783055/diff/2002/src/hydrogen.cc#newcode3974 src/hydrogen.cc:3974: HPhase phase("H_Apply actual values", this); It would be nice to be able to skip this completely when it is not needed (when there is no HBoundsCheck around). Feel free to fix that in a follow-up CL. https://chromiumcodereview.appspot.com/11783055/diff/2002/src/hydrogen.cc#newcode3981 src/hydrogen.cc:3981: if (phi->ActualValue() != phi) { This can never be true, right? Let's not do the work, then. If you want to be extra careful, you can put in a guard: #ifdef DEBUG for (...) { // A phi's actual value is currently always itself. HPhi* phi = block->phis()->at(i); ASSERT(phi->ActualValue() == phi); } #endif https://chromiumcodereview.appspot.com/11783055/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
