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.cc#newcode183
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.cc#newcode192
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.cc#newcode539
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.cc#newcode549
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.cc#newcode551
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.cc#newcode556
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.cc#newcode561
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-a64.cc
File src/a64/macro-assembler-a64.cc (right):
https://codereview.chromium.org/160423002/diff/160001/src/a64/macro-assembler-a64.cc#newcode2525
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-a64.cc#newcode2548
src/a64/macro-assembler-a64.cc:2548: bind(&done);
On 2014/02/13 14:20:14, jbramley wrote:
Bind(&done);
Done.
https://codereview.chromium.org/160423002/
--
--
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.
