Thanks for the review.
I addressed your comments and created an issue (2038) for the
implementation for
other archs.
http://code.google.com/p/v8/issues/detail?id=2038
The updated patch should be uploaded soon.
Alexandre
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();
Done.
This mechanism can be reintroduced if further optimizations do not
require to compute the remainder.
On 2012/03/28 09:59:18, fschneider wrote:
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:
On 2012/03/28 09:59:18, fschneider wrote:
#ifdef DEBUG
Done.
http://codereview.chromium.org/9638018/diff/1/src/arm/macro-assembler-arm.cc#newcode3733
src/arm/macro-assembler-arm.cc:3733:
On 2012/03/28 09:59:18, fschneider wrote:
#endif // DEBUG
Done.
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,
On 2012/03/28 09:59:18, fschneider wrote:
If this is only used in ASSERTs you can put it inside a
#ifdef DEBUG
Done.
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()) {
On 2012/03/28 09:59:18, fschneider wrote:
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.
Done.
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.
On 2012/03/28 09:59:18, fschneider wrote:
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.
Done.
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());
On 2012/03/28 09:59:18, fschneider wrote:
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.
Done.
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; }
On 2012/03/28 09:59:18, fschneider wrote:
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.
Done.
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++) {
On 2012/03/28 09:59:18, fschneider wrote:
Maybe use a different letter (j, k?), since 1 and l look to similar.
Done.
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));
On 2012/03/28 09:59:18, fschneider wrote:
Please break the line at 80 chars.
Done.
http://codereview.chromium.org/9638018/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
