Sorry for the long delay. Here are my comments: The approach looks good.
http://codereview.chromium.org/9638018/diff/1/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/9638018/diff/1/src/arm/lithium-codegen-arm.cc#newcode1047 src/arm/lithium-codegen-arm.cc:1047: bool compute_remainder = remainder.is_valid(); Isn't remainder always a temp-register and compute_remainder always true in here? In this case you could simplify the code a bit. http://codereview.chromium.org/9638018/diff/1/src/arm/macro-assembler-arm.cc File src/arm/macro-assembler-arm.cc (right): http://codereview.chromium.org/9638018/diff/1/src/arm/macro-assembler-arm.cc#newcode3712 src/arm/macro-assembler-arm.cc:3712: #ifdef DEBUG http://codereview.chromium.org/9638018/diff/1/src/arm/macro-assembler-arm.cc#newcode3733 src/arm/macro-assembler-arm.cc:3733: #endif // DEBUG http://codereview.chromium.org/9638018/diff/1/src/arm/macro-assembler-arm.h File src/arm/macro-assembler-arm.h (right): http://codereview.chromium.org/9638018/diff/1/src/arm/macro-assembler-arm.h#newcode88 src/arm/macro-assembler-arm.h:88: bool AreAliased(Register reg1, If this is only used in ASSERTs you can put it inside a #ifdef DEBUG http://codereview.chromium.org/9638018/diff/1/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): http://codereview.chromium.org/9638018/diff/1/src/hydrogen-instructions.cc#newcode917 src/hydrogen-instructions.cc:917: if (value()->IsDiv() && LChunkBuilder::SupportsMathFloorOfDiv()) { The function SupportsMathFloorDiv is essentially like writing #ifdef V8_TARGET_ARCH_ARM Maybe just put this part under the #ifdef and add a TODO(issue#) to add this for other platforms. e.g. Math.floor(x/4) can also be optimized for ia32. http://codereview.chromium.org/9638018/diff/1/src/hydrogen-instructions.cc#newcode946 src/hydrogen-instructions.cc:946: // If the division had no other uses than this HMathFloor, delete it. Not sure how often the division acutally has other uses, but I'd prefer to simplify the code here to restrict the optimization to the pattern Math.floor(a/b) where the division has exactly one use. http://codereview.chromium.org/9638018/diff/1/src/hydrogen-instructions.h File src/hydrogen-instructions.h (right): http://codereview.chromium.org/9638018/diff/1/src/hydrogen-instructions.h#newcode2654 src/hydrogen-instructions.h:2654: set_representation(Representation::Integer32()); I think you can make GVN handle this instruction by setting the flag SetFlag(kUseGVN) and implementing the DataEquals-function like for other arithmetic operations. In this case DataEquals would just return true. http://codereview.chromium.org/9638018/diff/1/src/ia32/lithium-ia32.h File src/ia32/lithium-ia32.h (right): http://codereview.chromium.org/9638018/diff/1/src/ia32/lithium-ia32.h#newcode2344 src/ia32/lithium-ia32.h:2344: static bool SupportsMathFloorOfDiv() { return false; } I'd add those function only when we add this optimization to ia32. By using a #ifdef in hydrogen-instructions.cc for the time being we can avoid adding unused code to the other platforms. http://codereview.chromium.org/9638018/diff/1/test/mjsunit/math-floor-of-div.js File test/mjsunit/math-floor-of-div.js (right): http://codereview.chromium.org/9638018/diff/1/test/mjsunit/math-floor-of-div.js#newcode52 test/mjsunit/math-floor-of-div.js:52: for (var l = 0; l <= limit; l++) { Maybe use a different letter (j, k?), since 1 and l look to similar. http://codereview.chromium.org/9638018/diff/1/test/mjsunit/math-floor-of-div.js#newcode54 test/mjsunit/math-floor-of-div.js:54: assertEquals(Math.floor(div(l, 1)), Math.floor(l / 1)); assertEquals(Math.floor(div(l, -1)), Math.floor(l / -1)); Please break the line at 80 chars. http://codereview.chromium.org/9638018/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
