Issue 18612005: Implement truncated d-to-i with a stub on x86

Issue 18612005: Implement truncated d-to-i with a stub on x86 (Closed)

Created:
7 years, 5 months ago by danno

Modified:
7 years, 5 months ago

Reviewers:
Yang

CC:
v8-dev, Jakob Kummerow

Base URL:
https://v8.googlecode.com/svn/branches/bleeding_edge

Visibility:
Public.

More Reviews

Description

Implement truncated d-to-i as a stub on x86 - Added a general DoubleToIStub so that it's possible to extend to other platforms and non-truncating case. - This version handles all cases of truncation (previous code deopted in some cases) and all source/destination register combinations without clobbering any temps. R=yangguo@chromium.org Committed: http://code.google.com/p/v8/source/detail?r=15645

Patch Set 1 #

Patch Set 2 : Refinements #

Patch Set 3 : Add tests #

Patch Set 4 : Add .js tests #

Patch Set 5 : Change CHECK to ASSERT #

Patch Set 6 : Add missing USE #

Patch Set 7 : Add support for all register combinations #

Total comments: 34

Patch Set 8 : Fixes #

Patch Set 9 : Review feedback #

Patch Set 10 : Merge with ToT #

Created: 7 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+552 lines, -199 lines)			Patch
M	src/code-stubs.h	View	1 2 3 4 5 6 7 8 9	2 chunks	+55 lines, -0 lines	0 comments	Download
M	src/ia32/code-stubs-ia32.cc	View	1 2 3 4 5 6 7 8 9	3 chunks	+134 lines, -118 lines	0 comments	Download
M	src/ia32/lithium-codegen-ia32.cc	View	1 2 3 4 5 6 7 8 9	1 chunk	+10 lines, -81 lines	0 comments	Download
M	test/cctest/cctest.gyp	View	1 2 3 4 5 6 7 9	1 chunk	+1 line, -0 lines	0 comments	Download
A	test/cctest/test-code-stubs-ia32.cc	View	1 2 3 4 5 6 7 8	1 chunk	+274 lines, -0 lines	0 comments	Download
A	test/mjsunit/double-truncation.js	View	1 2 3	1 chunk	+78 lines, -0 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

Yang

I haven't looked at the test case yet, but I got a bunch of comments ...

7 years, 5 months ago (2013-07-11 12:00:27 UTC) #2

danno

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 ...

7 years, 5 months ago (2013-07-11 16:00:23 UTC) #3

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....
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....
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....
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....
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....
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....
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....
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....
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....
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....
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....
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....
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....
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....
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....
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....
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-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/18612005/diff/15001/src/ia32/lithium-codegen-...
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.

Yang

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 ...

7 years, 5 months ago (2013-07-12 12:15:33 UTC) #4

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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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....
> 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-...
> File src/ia32/lithium-codegen-ia32.cc (right):
> 
>
https://codereview.chromium.org/18612005/diff/15001/src/ia32/lithium-codegen-...
> 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.

danno

7 years, 5 months ago (2013-07-12 12:28:40 UTC) #5

Message was sent while issue was closed.

Committed patchset #10 manually as r15645 (presubmit successful).

Expand Messages | Collapse Messages