I'm very sorry that this has taken this long. I had a look over it. This
looks
good so far, but I have some comments.
Also, please rebase this CL, there has been some changes related to passing
the
Zone explicitly.
http://codereview.chromium.org/10382033/diff/7011/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/10382033/diff/7011/src/ia32/lithium-codegen-ia32.cc#newcode985
src/ia32/lithium-codegen-ia32.cc:985: // FIXME: set & check
HValue::kBailoutOnMinusZero
Is this going to be fixed?
http://codereview.chromium.org/10382033/diff/7011/src/ia32/lithium-codegen-ia32.cc#newcode998
src/ia32/lithium-codegen-ia32.cc:998: // tedious because neg(dividend)
might overflow
Comments should start with capital letter and end with a period.
Wouldn't neg only overflow for -2^31? Can't we simply deoptimize for
this case and do a simple neg for the rest?
http://codereview.chromium.org/10382033/diff/7011/src/ia32/lithium-codegen-ia32.cc#newcode1021
src/ia32/lithium-codegen-ia32.cc:1021: double multiplier_f =
static_cast<double>((uint64_t)1 << shift)
Use static_cast please.
http://codereview.chromium.org/10382033/diff/7011/src/ia32/lithium-codegen-ia32.cc#newcode1030
src/ia32/lithium-codegen-ia32.cc:1030: ASSERT(multiplier > 0 &&
multiplier < ((int64_t)1<<32));
Use static_cast please.
http://codereview.chromium.org/10382033/diff/7011/src/ia32/lithium-codegen-ia32.cc#newcode1032
src/ia32/lithium-codegen-ia32.cc:1032: // FIXME: set & check
HValue::kBailoutOnMinusZero
Same here.
http://codereview.chromium.org/10382033/diff/7011/src/ia32/lithium-codegen-ia32.cc#newcode1039
src/ia32/lithium-codegen-ia32.cc:1039: if ((int32_t)multiplier < 0) {
Use static_cast please.
http://codereview.chromium.org/10382033/diff/7011/src/x64/lithium-codegen-x64.cc
File src/x64/lithium-codegen-x64.cc (right):
http://codereview.chromium.org/10382033/diff/7011/src/x64/lithium-codegen-x64.cc#newcode908
src/x64/lithium-codegen-x64.cc:908: // FIXME: set & check
HValue::kBailoutOnMinusZero
Any changes here?
http://codereview.chromium.org/10382033/diff/7011/src/x64/lithium-codegen-x64.cc#newcode922
src/x64/lithium-codegen-x64.cc:922: // FIXME: set & check
HValue::kBailoutOnMinusZero
Ditto.
http://codereview.chromium.org/10382033/diff/7011/src/x64/lithium-codegen-x64.cc#newcode938
src/x64/lithium-codegen-x64.cc:938: double multiplier_f =
static_cast<double>((uint64_t)1 << shift)
Use static_cast please.
http://codereview.chromium.org/10382033/diff/7011/src/x64/lithium-codegen-x64.cc#newcode947
src/x64/lithium-codegen-x64.cc:947: ASSERT(multiplier > 0 && multiplier
< ((int64_t)1<<32));
Use static_cast please.
http://codereview.chromium.org/10382033/diff/7011/src/x64/lithium-codegen-x64.cc#newcode952
src/x64/lithium-codegen-x64.cc:952: // FIXME: set & check
HValue::kBailoutOnMinusZero
Ditto.
http://codereview.chromium.org/10382033/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
