A64 support for DoubleToIStub (truncating).

A64 support for DoubleToIStub (truncating).

Added support for truncating DoubleToIStub and reorganize the macro-assembler
dToI operations to do the fast-path inline and the slow path by calling the
stub.

This a port essentially a port of https://codereview.chromium.org/23129003/.

R=jacob.bramley@arm.com, ulan@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=19414

[rmcilroy, 2014-02-13 08:17:26]

PTAL.

[jbramley, 2014-02-13 14:20:14]

https://codereview.chromium.org/160423002/diff/160001/src/a64/assembler-a64.cc
File src/a64/assembler-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/assembler-a64.c...
src/a64/assembler-a64.cc:183: Register GetRegisterThatIsNotOneOf(Register reg1,
Register reg2,
I'd call it GetAllocatableRegisterThatIsNotOneOf(...).

https://codereview.chromium.org/160423002/diff/160001/src/a64/assembler-a64.c...
src/a64/assembler-a64.cc:192: if (reg6.IsValid()) regs |= reg6.Bit();
CPURegList can do a lot of this for you:

CPURegList regs(reg1, reg2, ...);

for (...) {
  if (regs.IncludesAliasOf(candidate)) continue;
}

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:539: void DoubleToIStub::Generate(MacroAssembler*
masm) {
It seems that this stub doesn't always truncate on other architectures. We
should ASSERT(is_truncating()) somewhere.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:549: if (input.is(jssp)) double_offset += 2 *
kPointerSize;
Do this near Push(scratch1, scratch2) so it's clear what it's for.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:551: DoubleRegister double_scratch = d0;  // only used
if !skip_fastpath()
In that case, put it in the !skip_fastpath() scope.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:556: __ Push(scratch1, scratch2);
This is probably the only stub that preserves its registers. It took us a little
while to realise that this didn't need to be marked as a call. Could you add a
comment to the call site please?

Also, it would be more efficient to ask for temp registers than to
unconditionally preserve the scratch registers. Is there any way to arrange
that? Even using DefineFixed would be better than unconditionally preserving
them on the stack.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:561: __ Push(double_scratch);
If you could combine this with the previous Push, that would be nice. There is a
small overhead to every Push call.

https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler...
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.cc:2515: DoubleToIStub stub(jssp, result, 0, true,
true);
Please use comments to label those bools (or make them enums and update the
other back-ends accordingly).

https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.cc:2525: Sxtw(result, result);
Use Sxtw(result, result.W());

This enables a more strict assertion inside the assembler. Really, we shouldn't
allow Sxtw(xd, xn) because it doesn't make sense, but that's an issue we're
thinking about anyway.

https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.cc:2548: bind(&done);
Bind(&done);

https://codereview.chromium.org/160423002/diff/160001/test/cctest/test-code-s...
File test/cctest/test-code-stubs-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/test/cctest/test-code-s...
test/cctest/test-code-stubs-a64.cc:100: // // Make sure no registers have been
unexpectedly clobbered
Double comment.

https://codereview.chromium.org/160423002/diff/160001/test/cctest/test-code-s...
test/cctest/test-code-stubs-a64.cc:113: __ Mov(x0, destination_reg);
The 'if' isn't necessary; A64's Mov emits no code if the source and destination
are the same.

https://codereview.chromium.org/160423002/diff/160001/test/cctest/test-code-s...
test/cctest/test-code-stubs-a64.cc:137: int32_t
RunGeneratedCodeCallWrapper(ConvertDToIFunc func,
Does this need to return int64_t? If not, the result of CallInt64 should
probably be filtered through is_int32 or something similar.

[rmcilroy, 2014-02-14 12:45:07]

Thanks for the review. Comments below.

https://codereview.chromium.org/160423002/diff/160001/src/a64/assembler-a64.cc
File src/a64/assembler-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/assembler-a64.c...
src/a64/assembler-a64.cc:183: Register GetRegisterThatIsNotOneOf(Register reg1,
Register reg2,
On 2014/02/13 14:20:14, jbramley wrote:
> I'd call it GetAllocatableRegisterThatIsNotOneOf(...).

Done.

https://codereview.chromium.org/160423002/diff/160001/src/a64/assembler-a64.c...
src/a64/assembler-a64.cc:192: if (reg6.IsValid()) regs |= reg6.Bit();
On 2014/02/13 14:20:14, jbramley wrote:
> CPURegList can do a lot of this for you:
> 
> CPURegList regs(reg1, reg2, ...);
> 
> for (...) {
>   if (regs.IncludesAliasOf(candidate)) continue;
> }

Done.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:539: void DoubleToIStub::Generate(MacroAssembler*
masm) {
On 2014/02/13 14:20:14, jbramley wrote:
> It seems that this stub doesn't always truncate on other architectures. We
> should ASSERT(is_truncating()) somewhere.

Done (also on arm). Looks like the is_truncating() option isn't supported by any
arches, we should maybe just drop the option...

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:549: if (input.is(jssp)) double_offset += 2 *
kPointerSize;
On 2014/02/13 14:20:14, jbramley wrote:
> Do this near Push(scratch1, scratch2) so it's clear what it's for.

Done.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:551: DoubleRegister double_scratch = d0;  // only used
if !skip_fastpath()
On 2014/02/13 14:20:14, jbramley wrote:
> In that case, put it in the !skip_fastpath() scope.

I can't - I need to Pop it at the end of the function.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:556: __ Push(scratch1, scratch2);
On 2014/02/13 14:20:14, jbramley wrote:
> This is probably the only stub that preserves its registers. It took us a
little
> while to realise that this didn't need to be marked as a call. Could you add a
> comment to the call site please?

I'm not sure what you mean here? What would you mean by "marked as a call" - it
is called with CallStub, which typically doesn't respect caller/callee saved
register call semantics.  As far as I can tell RecordWriteStub does the same
thing (for scratch1_/scratch2_ and it doesn't have any special "marked as call"
or comment that I can see.
 
> Also, it would be more efficient to ask for temp registers than to
> unconditionally preserve the scratch registers. Is there any way to arrange
> that? Even using DefineFixed would be better than unconditionally preserving
> them on the stack.

If we allocated temp registers then we would need to allocate these every time
MacroAssembler::TruncateDoubleToI() was called, even though they would only
actually be used if we had to call into the stub (which is fairly infrequent
since most situations are successful with the inline code).  It's a trade off
between additional register pressure (for every TruncateDoubleToI) verses
additional push/pop overhead, but only in situations where we need to call out
to the stub.   When I did this for Arm there was no noticeable overhead increase
caused by the additions of the push/pops so I would prefer to do that here too
unless you strongly disagree.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:561: __ Push(double_scratch);
On 2014/02/13 14:20:14, jbramley wrote:
> If you could combine this with the previous Push, that would be nice. There is
a
> small overhead to every Push call.

I can't - Push() barfs that the registers are of different types (Register and
DoubleRegister).

https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler...
File src/a64/macro-assembler-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.cc:2525: Sxtw(result, result);
On 2014/02/13 14:20:14, jbramley wrote:
> Use Sxtw(result, result.W());
> 
> This enables a more strict assertion inside the assembler. Really, we
shouldn't
> allow Sxtw(xd, xn) because it doesn't make sense, but that's an issue we're
> thinking about anyway.

Done.

https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler...
src/a64/macro-assembler-a64.cc:2548: bind(&done);
On 2014/02/13 14:20:14, jbramley wrote:
> Bind(&done);

Done.

[jbramley, 2014-02-14 12:57:37]

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:539: void DoubleToIStub::Generate(MacroAssembler*
masm) {
On 2014/02/14 12:45:07, rmcilroy wrote:
> Looks like the is_truncating() option isn't supported by any
> arches, we should maybe just drop the option...

If we do, we should probably rename the stub too, but that's something to worry
about later.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:556: __ Push(scratch1, scratch2);
On 2014/02/14 12:45:07, rmcilroy wrote:
> On 2014/02/13 14:20:14, jbramley wrote:
> > This is probably the only stub that preserves its registers. It took us a
> little
> > while to realise that this didn't need to be marked as a call. Could you add
a
> > comment to the call site please?
> 
> I'm not sure what you mean here? What would you mean by "marked as a call" -
it
> is called with CallStub, which typically doesn't respect caller/callee saved
> register call semantics.  As far as I can tell RecordWriteStub does the same
> thing (for scratch1_/scratch2_ and it doesn't have any special "marked as
call"
> or comment that I can see.

At the moment, MarkAsCall preserves every live value (if I remember correctly),
even callee-saved values, because the call might do a GC.

Our stubs don't really have an ABI; we tend to be cautious and assume that they
can clobber anything except the stack. We inherited this from ARM, but we'd like
to improve this later.

It's perfectly Ok to define a stub that explicitly doesn't clobber any
registers, but it's unusual (for A64) so it needs a comment at the call site. If
I see CallStub, I normally think "clobber everything".

RecordWriteStub is a bit weird and I think I have some TODOs in there to clean
it up. At the moment, the underlying RecordWrite() overwrites some input
registers, for example.

> > Also, it would be more efficient to ask for temp registers than to
> > unconditionally preserve the scratch registers. Is there any way to arrange
> > that? Even using DefineFixed would be better than unconditionally preserving
> > them on the stack.
> 
> If we allocated temp registers then we would need to allocate these every time
> MacroAssembler::TruncateDoubleToI() was called, even though they would only
> actually be used if we had to call into the stub (which is fairly infrequent
> since most situations are successful with the inline code).  It's a trade off
> between additional register pressure (for every TruncateDoubleToI) verses
> additional push/pop overhead, but only in situations where we need to call out
> to the stub.   When I did this for Arm there was no noticeable overhead
increase
> caused by the additions of the push/pops so I would prefer to do that here too
> unless you strongly disagree.

That makes sense. I'd forgotten that the stub is actually the rare case, so the
impact of the push and pop is minimal anyway.

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:561: __ Push(double_scratch);
On 2014/02/14 12:45:07, rmcilroy wrote:
> I can't - Push() barfs that the registers are of different types (Register and
> DoubleRegister).

Ah, yes, sorry.

[rmcilroy, 2014-02-14 13:44:17]

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64.cc
File src/a64/code-stubs-a64.cc (right):

https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
src/a64/code-stubs-a64.cc:556: __ Push(scratch1, scratch2);
On 2014/02/14 12:57:38, jbramley wrote:
> On 2014/02/14 12:45:07, rmcilroy wrote:
> > On 2014/02/13 14:20:14, jbramley wrote:
> > > This is probably the only stub that preserves its registers. It took us a
> > little
> > > while to realise that this didn't need to be marked as a call. Could you
add
> a
> > > comment to the call site please?
> > 
> > I'm not sure what you mean here? What would you mean by "marked as a call" -
> it
> > is called with CallStub, which typically doesn't respect caller/callee saved
> > register call semantics.  As far as I can tell RecordWriteStub does the same
> > thing (for scratch1_/scratch2_ and it doesn't have any special "marked as
> call"
> > or comment that I can see.
> 
> At the moment, MarkAsCall preserves every live value (if I remember
correctly),
> even callee-saved values, because the call might do a GC.
> 
> Our stubs don't really have an ABI; we tend to be cautious and assume that
they
> can clobber anything except the stack. We inherited this from ARM, but we'd
like
> to improve this later.
> 
> It's perfectly Ok to define a stub that explicitly doesn't clobber any
> registers, but it's unusual (for A64) so it needs a comment at the call site.
If
> I see CallStub, I normally think "clobber everything".

Ok, done.  Just FYI - I expect we will be adding more of these "short
non-call-marked" stubs in the future since we are planning on doing some work to
avoid inlining exceptional cases (such as this) and move that work into stubs
instead to reduce code-bloat.


> RecordWriteStub is a bit weird and I think I have some TODOs in there to clean
> it up. At the moment, the underlying RecordWrite() overwrites some input
> registers, for example.
> 
> > > Also, it would be more efficient to ask for temp registers than to
> > > unconditionally preserve the scratch registers. Is there any way to
arrange
> > > that? Even using DefineFixed would be better than unconditionally
preserving
> > > them on the stack.
> > 
> > If we allocated temp registers then we would need to allocate these every
time
> > MacroAssembler::TruncateDoubleToI() was called, even though they would only
> > actually be used if we had to call into the stub (which is fairly infrequent
> > since most situations are successful with the inline code).  It's a trade
off
> > between additional register pressure (for every TruncateDoubleToI) verses
> > additional push/pop overhead, but only in situations where we need to call
out
> > to the stub.   When I did this for Arm there was no noticeable overhead
> increase
> > caused by the additions of the push/pops so I would prefer to do that here
too
> > unless you strongly disagree.
> 
> That makes sense. I'd forgotten that the stub is actually the rare case, so
the
> impact of the push and pop is minimal anyway.

[rmcilroy, 2014-02-17 09:55:00]

On 2014/02/14 13:44:17, rmcilroy wrote:
>
https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64.cc
> File src/a64/code-stubs-a64.cc (right):
> 
>
https://codereview.chromium.org/160423002/diff/160001/src/a64/code-stubs-a64....
> src/a64/code-stubs-a64.cc:556: __ Push(scratch1, scratch2);
> On 2014/02/14 12:57:38, jbramley wrote:
> > On 2014/02/14 12:45:07, rmcilroy wrote:
> > > On 2014/02/13 14:20:14, jbramley wrote:
> > > > This is probably the only stub that preserves its registers. It took us
a
> > > little
> > > > while to realise that this didn't need to be marked as a call. Could you
> add
> > a
> > > > comment to the call site please?
> > > 
> > > I'm not sure what you mean here? What would you mean by "marked as a call"
-
> > it
> > > is called with CallStub, which typically doesn't respect caller/callee
saved
> > > register call semantics.  As far as I can tell RecordWriteStub does the
same
> > > thing (for scratch1_/scratch2_ and it doesn't have any special "marked as
> > call"
> > > or comment that I can see.
> > 
> > At the moment, MarkAsCall preserves every live value (if I remember
> correctly),
> > even callee-saved values, because the call might do a GC.
> > 
> > Our stubs don't really have an ABI; we tend to be cautious and assume that
> they
> > can clobber anything except the stack. We inherited this from ARM, but we'd
> like
> > to improve this later.
> > 
> > It's perfectly Ok to define a stub that explicitly doesn't clobber any
> > registers, but it's unusual (for A64) so it needs a comment at the call
site.
> If
> > I see CallStub, I normally think "clobber everything".
> 
> Ok, done.  Just FYI - I expect we will be adding more of these "short
> non-call-marked" stubs in the future since we are planning on doing some work
to
> avoid inlining exceptional cases (such as this) and move that work into stubs
> instead to reduce code-bloat.
> 
> 
> > RecordWriteStub is a bit weird and I think I have some TODOs in there to
clean
> > it up. At the moment, the underlying RecordWrite() overwrites some input
> > registers, for example.
> > 
> > > > Also, it would be more efficient to ask for temp registers than to
> > > > unconditionally preserve the scratch registers. Is there any way to
> arrange
> > > > that? Even using DefineFixed would be better than unconditionally
> preserving
> > > > them on the stack.
> > > 
> > > If we allocated temp registers then we would need to allocate these every
> time
> > > MacroAssembler::TruncateDoubleToI() was called, even though they would
only
> > > actually be used if we had to call into the stub (which is fairly
infrequent
> > > since most situations are successful with the inline code).  It's a trade
> off
> > > between additional register pressure (for every TruncateDoubleToI) verses
> > > additional push/pop overhead, but only in situations where we need to call
> out
> > > to the stub.   When I did this for Arm there was no noticeable overhead
> > increase
> > > caused by the additions of the push/pops so I would prefer to do that here
> too
> > > unless you strongly disagree.
> > 
> > That makes sense. I'd forgotten that the stub is actually the rare case, so
> the
> > impact of the push and pop is minimal anyway.

Ping?

[jbramley, 2014-02-17 10:32:39]

I have one last question!

https://codereview.chromium.org/160423002/diff/340001/test/cctest/test-assemb...
File test/cctest/test-assembler-a64.cc (left):

https://codereview.chromium.org/160423002/diff/340001/test/cctest/test-assemb...
test/cctest/test-assembler-a64.cc:9377: TEST(ecma_262_to_int32) {
Does this test not work any more? I don't think I've ever tried to create a stub
in one of these tests.

Also, can you check that RunAllTruncationTests covers all of these cases please?
There is little intersection between the two sets of values, but the values here
were designed to exercise the A64 implementation. All of these conversions are
specified by ECMA so they should be valid for all platforms.

[rmcilroy, 2014-02-17 13:32:23]

On 2014/02/17 10:32:39, jbramley wrote:
> I have one last question!
> 
>
https://codereview.chromium.org/160423002/diff/340001/test/cctest/test-assemb...
> File test/cctest/test-assembler-a64.cc (left):
> 
>
https://codereview.chromium.org/160423002/diff/340001/test/cctest/test-assemb...
> test/cctest/test-assembler-a64.cc:9377: TEST(ecma_262_to_int32) {
> Does this test not work any more? I don't think I've ever tried to create a
stub
> in one of these tests.

Yes these tests don't work if you have to call into a stub (the branch to stub
address instruction doesn't get correctly relocated unless Code::CopyFrom is
called, which doesn't happen here). 
 
> Also, can you check that RunAllTruncationTests covers all of these cases
please?
> There is little intersection between the two sets of values, but the values
here
> were designed to exercise the A64 implementation. All of these conversions are
> specified by ECMA so they should be valid for all platforms.

Done (re-formatted the values to be similar to the others in
RunAllTruncationTests and be able to be copy/pasted into JS).

[jbramley, 2014-02-17 13:34:17]

lgtm

Fantastic, thanks!

[ulan, 2014-02-17 13:56:42]

lgtm (rubber-stamp)

[rmcilroy, 2014-02-17 15:10:00]

Committed patchset #7 manually as r19414 (presubmit successful).