Rico, thanks for the review!
Rodolph, thanks for the additional comments! Could you please take another
look
at the ARM code?
-- Vitaly
http://codereview.chromium.org/3446024/diff/11001/12001
File src/arm/stub-cache-arm.cc (right):
http://codereview.chromium.org/3446024/diff/11001/12001#newcode1699
src/arm/stub-cache-arm.cc:1699: __ sub(r0, r0, r1);
On 2010/09/27 14:17:23, Rodolph Perfetta wrote:
Did you mean sub(r0, r0, r1, SetCC) ?
Oops, fixed.
Also wouldn't the sequence:
cmp r0, #0
rsbmis r0, r0, #0
bmi slow
be better for an integer abs (or did I miss a smi corner case?)
I changed the current approach to only use two instructions. You are
very much welcome to send me a patch with your approach if you think
it's better.
http://codereview.chromium.org/3446024/diff/11001/12001#newcode1720
src/arm/stub-cache-arm.cc:1720: __ tst(r1, r3);
On 2010/09/27 14:17:23, Rodolph Perfetta wrote:
merge the mov with the tst: tst(r1, Operand(HeapNumber::kSignMask)).
You will need to do the same for the eor further down.
Will the constant fit into the instruction? I was afraid of using the
constant pool more than necessary, but maybe it's not an issue.
http://codereview.chromium.org/3446024/diff/11001/12002
File src/ia32/stub-cache-ia32.cc (right):
http://codereview.chromium.org/3446024/diff/11001/12002#newcode2016
src/ia32/stub-cache-ia32.cc:2016: __ mov(ebx, FieldOperand(eax,
HeapNumber::kExponentOffset));
On 2010/09/27 13:18:23, Rico wrote:
You could potentially use an xmm register here and do (assume loaded
into xmm0):
pxor(xmm1, xmm1);
subsd(xmm1, xmm0)
and(xmm0, xmm1)
Sure, I could do branchless floating point code as well, but I want to
avoid allocation in case the number is not negative already.
http://codereview.chromium.org/3446024/diff/11001/12006
File test/mjsunit/math-abs.js (right):
http://codereview.chromium.org/3446024/diff/11001/12006#newcode28
test/mjsunit/math-abs.js:28: // Flags: --max-new-space-size=262144
On 2010/09/27 13:18:23, Rico wrote:
Why this flag?
I want to test GC effects and so want GC to happen earlier.
http://codereview.chromium.org/3446024/diff/11001/12006#newcode78
test/mjsunit/math-abs.js:78: assertEquals(two_30 - 1, Math.abs(-two_30 +
1));
On 2010/09/27 13:18:23, Rico wrote:
Maybe add a few test cases where you call Math.abs on objects, arrays
and
strings that are not actual numbers (to test that we do indeed bail
out if this
is the case)
Good idea. Done.
http://codereview.chromium.org/3446024/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
