I agree with your comments. It is great if you can fix them and land.
Thanks, Alexandre http://codereview.chromium.org/9638018/diff/13001/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): http://codereview.chromium.org/9638018/diff/13001/src/arm/lithium-arm.cc#newcode1358 src/arm/lithium-arm.cc:1358: HConstant::cast(dividend)->HasInteger32Value()) { In short: yes we should remove it. Long: Divisions are folded at hydrogen generation time. From what I remember I found out when testing that in rare cases later optimizations stages could yield this case again. In a previous patch not handling this case would raised an error. Now we would just miss an optimization if we don't handle it. I ran the tests quickly and did not see this situation occuring any more. So it seems we should really remove it. On 2012/04/17 07:41:10, fschneider wrote:
This would only occur is in Math.floor(a/b) both a and b are
constants. In this
case the division would be folded already, right? So you can probably
leave out
this case here.
http://codereview.chromium.org/9638018/diff/13001/test/mjsunit/math-floor-of-div.js File test/mjsunit/math-floor-of-div.js (right): http://codereview.chromium.org/9638018/diff/13001/test/mjsunit/math-floor-of-div.js#newcode169 test/mjsunit/math-floor-of-div.js:169: // (values[key] | 0) does not work anymore as it is optimized by hydrogen.) I remember changing to '+0' instead of '|0' when the latter was optimized in hydrogen. From what I remember the representation for the value loaded from the array was not forced to int32 and could stay tagged as a smi, preventing the opt for Math.floor(int32/int32). Maybe I mis-tested, anyway I re-tested and '|0'works fine. On 2012/04/17 07:41:10, fschneider wrote:
I don't understand why +0 is necessary. It's fine, but maybe also
optimized away
in the future.
x|0 is optimized into x, but x is still forced to be integer32
because of the
bitwise semantics. Maybe you can still use |0 here?
http://codereview.chromium.org/9638018/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
