Sorry to re-open this up, but I have some questions/comments after being
able to
think about this for a while.
Also, can you also please add unit tests that ensure coverage of the odd
edge
cases like x/0, kMinInt / -1 and -0 result with and without crankshaft? You
should add two copies of the test, a "normal" one and one that passes the
enable_sudiv flag in the flags header to make sure that the SDIV path gets
executed?
https://codereview.chromium.org/11316105/diff/5001/src/arm/lithium-codegen-arm.cc
File src/arm/lithium-codegen-arm.cc (right):
https://codereview.chromium.org/11316105/diff/5001/src/arm/lithium-codegen-arm.cc#newcode1341
src/arm/lithium-codegen-arm.cc:1341: if
(instr->right()->IsConstantOperand()) {
Can you please make sure the comment about "division by constants"
remains and is adjusted appropriately?
https://codereview.chromium.org/11316105/diff/5001/src/arm/lithium-codegen-arm.cc#newcode1360
src/arm/lithium-codegen-arm.cc:1360: __ sdiv(result, left, scratch);
from here to the sub(result, result, ....) the code is the same as in
the SDIV-specific else below. If you add a return above after the
EmitSignedIntegerDivisionByConstant, you can share this common code
https://codereview.chromium.org/11316105/diff/5001/src/arm/lithium-codegen-arm.cc#newcode1364
src/arm/lithium-codegen-arm.cc:1364: // We performed a truncating
division. Correct the result if necessary.
Why is it safe to omit the x/0, kMinInt/-1 and -0 checks below (or
simplified forms of them) which can still occur with a constant divisor
on this path?
https://codereview.chromium.org/11316105/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
