I haven't looked at the test case yet, but I got a bunch of comments
already.
And as we already discussed offline, there seems to be some problems with
compiling the test with g++.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode645
src/ia32/code-stubs-ia32.cc:645: }
Seems easy to introduce bugs in here.
How about something like
Register scratch1;
{
Register scratch_candidates[3] = { ebx, edx, edi };
for (int i = 0; i < 3; i++) {
scratch1 = scratch_candidates[i];
if (!final_result_reg.is(scratch1) && input_reg.is(scratch1))
break;
}
}
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode647
src/ia32/code-stubs-ia32.cc:647: Register save_reg =
final_result_reg.is(ecx) ? eax : ecx;
why do we need to spill eax if the result is expected in ecx? Just so
that the esp offset is 3 stack slots?
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode665
src/ia32/code-stubs-ia32.cc:665: }
At this point, we have overwritten ecx regardless what input_reg is.
Couldn't we just always load the mantissa onto the FP stack first and
then overwrite ecx with the exponent?
Unless you expect performance difference by loading the exponent
earlier, in which case, ignore this comment.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode668
src/ia32/code-stubs-ia32.cc:668: Immediate(HeapNumber::kExponentMask >>
HeapNumber::kExponentShift));
Looks like we could first mask and then shift, like we do elsewhere, so
that the mask doesn't have to be shifted. Easier to read imo.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode669
src/ia32/code-stubs-ia32.cc:669: __ lea(result_reg, MemOperand(ecx,
-HeapNumber::kExponentBias));
As far as I can see the value in result_reg is not used later on. We
could just compare ecx to (HeapNumber::kExponentBias +
HeapNumber::kMantissaBits) directly, without lea.
Or even use sub instead of cmp, which stores the result in ecx for later
and also sets the flags for the following conditional jump. In line
702-704 we would then simply neg(ecx).
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode679
src/ia32/code-stubs-ia32.cc:679: __ mov(result_reg, Immediate(0));
Use Set instead of mov to use xor, which is shorter. Or just use xor
inline.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode689
src/ia32/code-stubs-ia32.cc:689: // Already a copy of the mantissa on
the stack, overwrite it.
Comment seems wrong. We pushed a copy of the exponent onto the stack,
not of the mantissa.
We also should, at some point, add a STATIC_ASSERT(kDoubleSize == 2 *
kPointerSize), since we do rely on that.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode698
src/ia32/code-stubs-ia32.cc:698: __ add(esp,
Immediate(sizeof(uint64_t))); // Nolint.
We should use kDoubleSize for consistency. Also, do we really need
Nolint?
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode704
src/ia32/code-stubs-ia32.cc:704: __ mov(ecx, result_reg);
sub(ecx, Immediate(delta));
neg(ecx);
should do the same?
Alternatively, the sub has already been done as described above, so we
just need a neg.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode722
src/ia32/code-stubs-ia32.cc:722: __ j(equal, &skip_mov);
mark as near jump.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode742
src/ia32/code-stubs-ia32.cc:742: __ j(less_equal, &skip_mov);
mark as near jump
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode753
src/ia32/code-stubs-ia32.cc:753: if (final_result_reg.is(ecx)) __
mov(ecx, save_reg);
I think something like
if (!final_result_reg.is(result_reg)) {
ASSERT(final_result_reg.is(ecx));
__ mov(final_result_reg, result_reg);
}
is easier to understand.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode983
src/ia32/code-stubs-ia32.cc:983: DoubleToIStub stub(eax, ecx,
HeapNumber::kValueOffset - kSmiTagSize, true);
I think you want to use (HeapNumber::kValueOffset - kHeapObjectTag).
Even though both equal to 1.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode1016
src/ia32/code-stubs-ia32.cc:1016: DoubleToIStub stub(edx, ecx,
HeapNumber::kValueOffset - kSmiTagSize, true);
Ditto.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode2697
src/ia32/code-stubs-ia32.cc:2697: DoubleToIStub stub(edx, ecx,
HeapNumber::kValueOffset - kSmiTagSize, true);
Ditto.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode2733
src/ia32/code-stubs-ia32.cc:2733: DoubleToIStub stub(eax, ecx,
HeapNumber::kValueOffset - kSmiTagSize, true);
Ditto.
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://chromiumcodereview.appspot.com/18612005/diff/15001/src/ia32/lithium-codegen-ia32.cc#newcode5514
src/ia32/lithium-codegen-ia32.cc:5514: __ cvttsd2si(result_reg,
Operand(input_reg));
I wonder why this isn't put into the DoubleToIStub. Loading a double
onto an xmm register should be at least as efficient as loading it onto
the FP stack. And we save a step if it's already in an xmm register,
like is the case here. We wouldn't have to put the double value from xmm
register onto the FP stack via the stack anymore.
If you are worried about the additional call to get to the stub, we
could create a macro assembler function that tries this fast case first
before going slow case through the stub.
https://chromiumcodereview.appspot.com/18612005/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.
