Looks good overall. I have some suggestions for simplification.
https://chromiumcodereview.appspot.com/10382055/diff/1/src/flag-definitions.h File src/flag-definitions.h (right): https://chromiumcodereview.appspot.com/10382055/diff/1/src/flag-definitions.h#newcode200 src/flag-definitions.h:200: DEFINE_bool(array_index_dehoisting, true, Is there a reason to keep the flag around? https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h#newcode3992 src/hydrogen-instructions.h:3992: uint32_t index_offset_; Why is this a uint32_t? In hydrogen.cc, you're passing in an int32_t that indeed can be negative. Please use consistently signed variable types. (Same below.) https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen-instructions.h#newcode3998 src/hydrogen-instructions.h:3998: HLoadKeyedFastDoubleElement(HValue* elements, HValue* key) : nit: the colon goes in the next line (see HLoadKeyedFastElement as an example) https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc File src/hydrogen.cc (right): https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3122 src/hydrogen.cc:3122: // FIXME(mmassi): decide meaningful bound fix this before landing? :-) https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3127 src/hydrogen.cc:3127: counters->array_indexes_removed()->Increment(); Is it useful to keep those counters, or have they served their purpose? https://chromiumcodereview.appspot.com/10382055/diff/1/src/hydrogen.cc#newcode3143 src/hydrogen.cc:3143: if (instr->IsLoadKeyedFastElement()) { This is a lot of duplicate code... Suggestion: class ArrayInstructionInterface { public: virtual HInstruction* key() = 0; virtual int32_t key_position { return 1; } virtual void SetIndexOffset(int32_t index_offset) = 0; } Then you can let H{Load,Store}Keyed{Fast,FastDouble,SpecializedArray}Element inherit that and make DehoistArrayIndex() a whole lot easier (or even inline it into its single remaining call site). The downside would be that those three virtual methods can no longer be inlined, but I think they're not that performance critical. https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2433 src/ia32/lithium-codegen-ia32.cc:2433: instr->additional_index())); You can use "instr->hydrogen()->index_offset()" here. (Same below.) https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2458 src/ia32/lithium-codegen-ia32.cc:2458: instr->elements(), Leaving these on one line is fine, actually. But if you prefer, you can split them. https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode2472 src/ia32/lithium-codegen-ia32.cc:2472: uint32_t additional_index) { int32_t https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.cc#newcode3485 src/ia32/lithium-codegen-ia32.cc:3485: - kHeapObjectTag, Nit: this is ugly, please move all parameters to the next line, indented by 4 spaces. https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.h File src/ia32/lithium-codegen-ia32.h (right): https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-codegen-ia32.h#newcode246 src/ia32/lithium-codegen-ia32.h:246: uint32_t additional_index = 0); int32_t please. https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.cc File src/ia32/lithium-ia32.cc (right): https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.cc#newcode1964 src/ia32/lithium-ia32.cc:1964: instr->index_offset()); Again, I think you don't need any of the changes to this file. https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): https://chromiumcodereview.appspot.com/10382055/diff/1/src/ia32/lithium-ia32.h#newcode1250 src/ia32/lithium-ia32.h:1250: uint32_t additional_index_; You don't need to pass this value to the LInstruction. Just use the hydrogen() accessor to fetch it from the associated HInstruction. I think that obsoletes all of the changes to this file. https://chromiumcodereview.appspot.com/10382055/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
