PTAL
http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.cc#newcode1715 src/hydrogen-instructions.cc:1715: if (IsFastHoleyElementsKind(elements_kind())) { On 2012/06/12 08:10:43, Michael Starzinger wrote:
It seems as if we don't need the hole_check_mode flag for this
instruction
anymore, because it solely depends on the elements kind. Can we get
rid of the
flag, move the IsFastHoleyElementsKind() into a HLoadKeyedFastElements::MightHaveHoles() helper method and use this
helper
method here?
Done. http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.cc#newcode1722 src/hydrogen-instructions.cc:1722: if (IsFastPackedElementsKind(elements_kind())) { The hole check value is gone, but we still need to answer the question by implementing this function, so this code still seems necessary. On 2012/06/12 08:10:43, Michael Starzinger wrote:
Likewise.
http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.h#newcode622 src/hydrogen-instructions.h:622: virtual bool IsValueTaggedSmi() const { return false; } No, unfortunately not. The representation is tagged, not integer. What this method indicates additionally that it is always a SMI, and that can't be expressed with the representation. On 2012/06/12 08:10:43, Michael Starzinger wrote:
Can't we use HValue::CalculateInferredType instead of this new method?
http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.h#newcode1853 src/hydrogen-instructions.h:1853: bool result_is_smi) On 2012/06/12 08:10:43, Michael Starzinger wrote:
Can we use an enum instead of a bool flag here?
Done. http://codereview.chromium.org/10543094/diff/6001/src/hydrogen-instructions.h#newcode1875 src/hydrogen-instructions.h:1875: virtual bool IsValueTaggedSmi() const { See my reply above. On 2012/06/12 08:10:43, Michael Starzinger wrote:
See first comment in this file.
http://codereview.chromium.org/10543094/diff/6001/src/ia32/lithium-codegen-ia32.cc File src/ia32/lithium-codegen-ia32.cc (right): http://codereview.chromium.org/10543094/diff/6001/src/ia32/lithium-codegen-ia32.cc#newcode3883 src/ia32/lithium-codegen-ia32.cc:3883: #if DEBUG On 2012/06/12 09:01:13, Florian Schneider wrote:
We usually use FLAG_debug_code to emit assertion in generated code so
that the
generated code in debug and release mode is the same by default.
Done. http://codereview.chromium.org/10543094/diff/6001/src/ia32/lithium-codegen-ia32.cc#newcode3888 src/ia32/lithium-codegen-ia32.cc:3888: __ bind(&ok); On 2012/06/12 09:01:13, Florian Schneider wrote:
You could replace this block of instructions with:
__ AbortIfNotSmi(ToRegister(input));
Done. http://codereview.chromium.org/10543094/diff/6001/test/mjsunit/fast-array-length.js File test/mjsunit/fast-array-length.js (right): http://codereview.chromium.org/10543094/diff/6001/test/mjsunit/fast-array-length.js#newcode28 test/mjsunit/fast-array-length.js:28: // Flags: --allow-natives-syntax --expose-gc On 2012/06/12 08:10:43, Michael Starzinger wrote:
It seems you don't need --expose-gc here.
Done. http://codereview.chromium.org/10543094/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
