On 2013/07/11 16:00:23, danno wrote:
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.
LGTM.
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.
