This is in general going in exactly the right direction. Biggest feedback: is_external is redundant when you have elements_kind. There are also several cases that can share even more code.
Please not that I just inspected and commented the ARM code, but I suspect my
comments apply to ia32 and x64, too. https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1875 src/arm/lithium-arm.cc:1875: if (instr->RequiresHoleCheck()) AssignEnvironment(result); make sure that AssignEnvironment is the last thing done for consistency. It should operate on the result of DefineAsRegister(result). https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1888 src/arm/lithium-arm.cc:1888: LOperand* key = UseRegisterOrConstant(instr->key()); This can be UseRegisterOrConstantAtStart, I think, too, so you can combine the key calculate. UseRegisterOrConstantAtStart just means that it can re-use the register passed in for the key as the output register, if I recall correctly, UseRegisterOrConstant doesn't. In the arm case (and other platforms, too), the load result and the key aren't live at the same time for any of the load variants, so it's OK to use UseRegisterOrConstantAtStart. https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1894 src/arm/lithium-arm.cc:1894: return (elements_kind == EXTERNAL_UNSIGNED_INT_ELEMENTS) ? You can combine this exit sequence with the one for non external elements: bool can_deoptimize = instr->RequiresHoleCheck() || elements_kin == EXTERNAL_UNSIGNED_INT_ELEMENTS; return can_deoptimize ? AssignEnvironment(result) : result; https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1953 src/arm/lithium-arm.cc:1953: : UseRegister(instr->value()); Use the previous four lines to compute val in both the external and non-external case. in the non external case, elements_kind won't be EXTERNAL_PIXEL_ELEMENTS or EXTERNAL_FLOAT_ELEMENTS, so you'll get a temporary register as expected. https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#newcode1954 src/arm/lithium-arm.cc:1954: LOperand* key = UseRegisterOrConstant(instr->key()); As in load case, use LOperand* key = UseRegisterOrConstantAtStart(instr->key()); for both branches. https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode2999 src/arm/lithium-codegen-arm.cc:2999: nit: two line returns between functions https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode3006 src/arm/lithium-codegen-arm.cc:3006: bool key_is_constant = instr->key()->IsConstantOperand(); Just for clarity and readability, perhaps it makes sense to factor this part of the if info a helper function "DoLoadKeyedFixedDoubleArray", just like you did for DoLoadKeyedExternal? https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode3044 src/arm/lithium-codegen-arm.cc:3044: Register result = ToRegister(instr->result()); Perhaps here too with DoLoadKeyedFixedArray? https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode4068 src/arm/lithium-codegen-arm.cc:4068: nit: two spaces between functions https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-arm.cc#newcode4074 src/arm/lithium-codegen-arm.cc:4074: DwVfpRegister value = ToDoubleRegister(instr->value()); Same here as above, how about DoStoreKeyedFixedDoubleArray and DoStoreKeyedFixedArray? https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1855 src/hydrogen-instructions.cc:1855: stream->Add("["); From the above line https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1861 src/hydrogen-instructions.cc:1861: } To here can be shared at the end of the routine. RequiresHoleCheck should always return false for external arrays, so it will work the way the external array code expects. https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1948 src/hydrogen-instructions.cc:1948: // fast double HLoadKeyed. Yes, this should be OK https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode2014 src/hydrogen-instructions.cc:2014: stream->Add("["); Can share from above line... https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode2017 src/hydrogen-instructions.cc:2017: value()->PrintNameTo(stream); ...to line above between both external and non-external cases. https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4258 src/hydrogen-instructions.h:4258: if (!is_external) { use IsExternalArrayElementsKind here https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4292 src/hydrogen-instructions.h:4292: bool is_external() const { return use IsExternalArrayElementsKind(elements_kind()) to calculate the result https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4345 src/hydrogen-instructions.h:4345: // Is this nessecary at all? We should be able to treat all array accesses uniformly, including optimizing away bounds checks on external array, so this code shouldn't be necessary. BTW: You should make sure that array bounds check elimination works for external arrays after your changes. https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4357 src/hydrogen-instructions.h:4357: return is_external() ? true : !RequiresHoleCheck(); RequiresHoleCheck should return false for external arrays, so IsDeletable should just return !RequiresHoleCheck() https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4497 src/hydrogen-instructions.h:4497: ElementsKind elements_kind, bool is_external) Same feedback here about is_external as in the load case... it should be computed from elements_kind https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4515 src/hydrogen-instructions.h:4515: // (<tagged> | <external>>[int32] = <tagged> | double | int nit: (<tagged> | <external>)[int32] = (<tagged> | double | int) https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.h#newcode4523 src/hydrogen-instructions.h:4523: ASSERT_EQ(index, 2); Perhaps add a new predicate in elements-kind.h that detects all floating/double types? e.g. create: inline bool IsDoubleOrFloatElementsKind(ElementsKind elements_kind) { return IsFastDoubleElementsKind(elements_kind) || elements_kind == EXTERNAL_DOUBLE_ELEMENTS || elements_kind == EXTERNAL_FLOAT_ELEMENTS; } then the rest of this function become: if (IsDoubleOrFloatElementsKind()) return Representation::Double(); return is_external() ? Representation::Tagged() : Representation::Integer32(); https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc#newcode3749 src/hydrogen.cc:3749: // What should be done about that? Why has the limitation changed? https://codereview.chromium.org/11238016/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
