I have still some comments, but I think after those are addressed, this CL
is
good to be landed.
http://codereview.chromium.org/10382033/diff/16002/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):
http://codereview.chromium.org/10382033/diff/16002/src/hydrogen-instructions.h#newcode2780
src/hydrogen-instructions.h:2780: virtual HValue*
EnsureAndPropagateNotMinusZero(BitVector* visited) {
All implementations of EnsureAndPropagateNotMinusZero can be found in
hydrogen-instructions.cc around like 2200. Please only put the function
header here and move the implementation to hydrogen-instructions.cc like
the others.
http://codereview.chromium.org/10382033/diff/16002/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
http://codereview.chromium.org/10382033/diff/16002/src/ia32/lithium-codegen-ia32.cc#newcode1058
src/ia32/lithium-codegen-ia32.cc:1058: // The sequence is tedious
because neg(dividend) might overflow.
I took a second look. Wouldn't we circumvent this problem if we simply
divide first and then negate? We would then observe three distinct cases
(if divisor < 0):
- divisor is -1: we can simply do a NEG. Only if divident is kMinInt
would NEG overflow. We guard against this case first and deopt here,
since we would require a double representation for the result anyway
(because (-kMinInt) > kMaxInt). This case has already been dealt with in
the switch!
- divisor is -2 or less, and divident is 0: we bail out if
kBailoutOnMinusZero is set, otherwise return 0.
- divisor is -2 or less, and divident is not 0: we calculate
divident/divisor_abs by shifting and set the sign with neg. We don't
expect overflow for neg because divident/divisor_abs cannot be kMinInt.
Did I overlook anything? With this, we don't clobber divident, so it
doesn't have to be reserved with UseTempRegister.
http://codereview.chromium.org/10382033/diff/16002/src/ia32/lithium-codegen-ia32.cc#newcode1080
src/ia32/lithium-codegen-ia32.cc:1080: unsigned b = 31 -
CompilerIntrinsics::CountLeadingZeros(divisor_abs);
I assume divisor_abs == divisor here? Maybe use the former here can
avoid some confusions.
http://codereview.chromium.org/10382033/diff/16002/test/mjsunit/math-floor-of-div.js
File test/mjsunit/math-floor-of-div.js (right):
http://codereview.chromium.org/10382033/diff/16002/test/mjsunit/math-floor-of-div.js#newcode217
test/mjsunit/math-floor-of-div.js:217:
Please create a new test file called math-floor-of-div-minus-zero.js,
and add it to mjsunit.status so that it is skipped on ARM and MIPS.
http://codereview.chromium.org/10382033/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
