Getting very close. Mostly nits. Using the extra bit for ElementsKind is fine, by the way.
https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-arm.cc#newcode3001 src/arm/lithium-codegen-arm.cc:3001: void LCodeGen::DoLoadKeyedDouble(LLoadKeyed* instr) { nit: Just to make searches easier, maybe call this DoLoadKeyedFixedDouble. Even better, call this DoLoadKeyedFixedDoubleArray and the other one DoLoadKeyedFixedArray https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions.h#newcode4285 src/hydrogen-instructions.h:4285: SetGVNFlag(kDependsOnCalls); Technically, I think the call to SetGVNFlag(kDependsOnCalls) isn't necessary, but that's a deeper discussion outside the scope of this CL. Ask it about me when you get back, it might be a second cleanup CL. https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions.h#newcode4302 src/hydrogen-instructions.h:4302: nit: remove whitespace change https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions.h#newcode4349 src/hydrogen-instructions.h:4349: // Is this okay? That's fine. But crate a constant for the number of bits you use and add a static assert here that makes sure that it is enough to hold ElementsKind, e.g. STATIC_ASSERT(kElementsKindCount <= (1 << kBitsForElementsKind)) https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-ia32.cc#newcode2866 src/ia32/lithium-codegen-ia32.cc:2866: nit: two spaces https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-ia32.cc#newcode3911 src/ia32/lithium-codegen-ia32.cc:3911: Register key = instr->key()->IsRegister() ? ToRegister(instr->key()) I think this still fits on one line? https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#newcode1395 src/ia32/lithium-ia32.h:1395: LOperand* object() { return inputs_[0]; } I think elements is actually better. No matter which variant, external or not, this value is the "base" of the elements that you need to index into. Object has a bit of a connotation that it's any HeapObject, or even a JavaScript object, which it's not. https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#newcode1981 src/ia32/lithium-ia32.h:1981: LOperand* object() { return inputs_[0]; } Change this one to elements() for symmetry with LStoreKeyed. https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h File src/x64/lithium-x64.h (right): https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h#newcode1365 src/x64/lithium-x64.h:1365: LOperand* object() { return inputs_[0]; } elements? https://codereview.chromium.org/11238016/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
