On 2012/06/20 13:54:15, Zheng Liu wrote:
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.
I see. LGTM. I'll land this CL. Thanks for sticking with me.
http://codereview.chromium.org/10382033/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
