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) { On 2012/06/20 12:19:42, Yang wrote:
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. Done. 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. On 2012/06/20 12:19:42, Yang wrote:
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.
As a floor() op, it's not symmetric around zero. e.g. Math.floor(1/-4) => -1 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); On 2012/06/20 12:19:42, Yang wrote:
I assume divisor_abs == divisor here? Maybe use the former here can
avoid some
confusions.
Here abs(divisor) is not-power-of-two. divisor could be negative. 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: On 2012/06/20 12:19:42, Yang wrote:
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.
Done. http://codereview.chromium.org/10382033/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
