Please take another look
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode645
src/ia32/code-stubs-ia32.cc:645: }
On 2013/07/11 12:00:27, Yang wrote:
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;
}
}
Done.
https://codereview.chromium.org/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;
On 2013/07/11 12:00:27, Yang wrote:
why do we need to spill eax if the result is expected in ecx? Just so
that the
esp offset is 3 stack slots?
No, it's because we use eax above to calculate the result above if the
the real result is ecx, and part of the contract is that we don't
clobber registers. I'll comment.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode665
src/ia32/code-stubs-ia32.cc:665: }
On 2013/07/11 12:00:27, Yang wrote:
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.
Done.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode668
src/ia32/code-stubs-ia32.cc:668: Immediate(HeapNumber::kExponentMask >>
HeapNumber::kExponentShift));
On 2013/07/11 12:00:27, Yang wrote:
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.
Done.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode669
src/ia32/code-stubs-ia32.cc:669: __ lea(result_reg, MemOperand(ecx,
-HeapNumber::kExponentBias));
On 2013/07/11 12:00:27, Yang wrote:
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).
This is actually a different test. The cmp and below branch tests a
range of values. You can't do that if you simply do the subtract and
check for less that zero.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode679
src/ia32/code-stubs-ia32.cc:679: __ mov(result_reg, Immediate(0));
On 2013/07/11 12:00:27, Yang wrote:
Use Set instead of mov to use xor, which is shorter. Or just use xor
inline.
Done.
https://codereview.chromium.org/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.
On 2013/07/11 12:00:27, Yang wrote:
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.
Done.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode698
src/ia32/code-stubs-ia32.cc:698: __ add(esp,
Immediate(sizeof(uint64_t))); // Nolint.
On 2013/07/11 12:00:27, Yang wrote:
We should use kDoubleSize for consistency. Also, do we really need
Nolint?
Done.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode704
src/ia32/code-stubs-ia32.cc:704: __ mov(ecx, result_reg);
On 2013/07/11 12:00:27, Yang wrote:
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.
Done.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode722
src/ia32/code-stubs-ia32.cc:722: __ j(equal, &skip_mov);
On 2013/07/11 12:00:27, Yang wrote:
mark as near jump.
Done.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/code-stubs-ia32.cc#newcode742
src/ia32/code-stubs-ia32.cc:742: __ j(less_equal, &skip_mov);
On 2013/07/11 12:00:27, Yang wrote:
mark as near jump
Done.
https://codereview.chromium.org/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);
On 2013/07/11 12:00:27, Yang wrote:
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.
Done.
https://codereview.chromium.org/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);
On 2013/07/11 12:00:27, Yang wrote:
I think you want to use (HeapNumber::kValueOffset - kHeapObjectTag).
Even though
both equal to 1.
Done.
https://codereview.chromium.org/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);
On 2013/07/11 12:00:27, Yang wrote:
Ditto.
Done.
https://codereview.chromium.org/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);
On 2013/07/11 12:00:27, Yang wrote:
Ditto.
Done.
https://codereview.chromium.org/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);
On 2013/07/11 12:00:27, Yang wrote:
Ditto.
Done.
https://codereview.chromium.org/18612005/diff/15001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):
https://codereview.chromium.org/18612005/diff/15001/src/ia32/lithium-codegen-ia32.cc#newcode5514
src/ia32/lithium-codegen-ia32.cc:5514: __ cvttsd2si(result_reg,
Operand(input_reg));
Well, the stub works on a buffer that points to the number, so I don't
think we can make it work mechanically to pass in a double register as
the source. You're right, DoubleToIStub can probably optimize with this
case, too, if available, but I'd like to save that change (and your
macro assembler change) for another CL).
On 2013/07/11 12:00:27, Yang wrote:
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://codereview.chromium.org/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.
