http://codereview.chromium.org/8002019/diff/1/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right):
http://codereview.chromium.org/8002019/diff/1/src/hydrogen-instructions.h#newcode3737 src/hydrogen-instructions.h:3737: ValueType value_type = GENERIC_VALUE) Why not just use ElementKind here? http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc File src/hydrogen.cc (right): http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode3342 src/hydrogen.cc:3342: HBasicBlock* store_fast = graph()->CreateBasicBlock(); Wow, cool, you went for bonus points here (I don't know if this is a perf bottneck or not). It's probably a good idea to include a unit test that explicitly triggers this logic, since it is now more than a one-liner. http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4007 src/hydrogen.cc:4007: HInstruction* HGraphBuilder::BuildFastElementAccess(HValue* elements, I find it a little asymmetric that BuildFirstElementAccess adds the SmiCheck, but not the last one. Maybe instead it should always to the AddInstruction for the result instruction, and return a HValue* (or NULL for stores)? http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4028 src/hydrogen.cc:4028: return new(zone()) HLoadKeyedFastElement(elements, checked_key); If you're refactoring anyway, perhaps you might want to kill the arrow anti-pattern? http://www.codinghorror.com/blog/2006/01/flattening-arrow-code.html http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4069 src/hydrogen.cc:4069: ASSERT(fast_smi_only_elements || fast_elements || fast_double_elements); Perhaps add predicates to the map that test for multiple values as we did for Object's HasXXXXElements, like fast_type_elements()? http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4158 src/hydrogen.cc:4158: // When the |object| has FAST_SMI_ONLY_ELEMENTS, chances are we'll stype nit: passive voice instead of "we"? http://codereview.chromium.org/8002019/diff/1/src/hydrogen.cc#newcode4168 src/hydrogen.cc:4168: // TODO(jkummerow): We could get around the need for these two blocks stype nit: passive voice instead of "we"? http://codereview.chromium.org/8002019/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
