Thanks for the review! I realise that the patch is rather large.
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc
File src/a64/macro-assembler-a64.cc (right):
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode2335
src/a64/macro-assembler-a64.cc:2335: Csel(output.W(), output.W(), 255,
le);
On 2014/02/24 11:04:49, rmcilroy wrote:
Could you pull out the behaviour change (changing gt to le etc.) into
a separate
CL please.
The behaviour is the same; I swapped the arguments because only the
second operand can take an immediate. I had to invert the condition code
accordingly. (When this code was written, Csel couldn't accept an
Operand so we had to pass in a register.)
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode2489
src/a64/macro-assembler-a64.cc:2489: masm_temps.Include(temps);
On 2014/02/24 11:04:49, rmcilroy wrote:
I actually think the old approach here of popping explicitly from
temps was
cleaner and more understandable (and would enable removal of The
Include
method). You could replace the methods below with something like:
CopyFieldsLoopPairsHelper(dst, src, count,
masm_temps.AquireX(),
masm_temps.AquireX(),
Register(extra_temps.PopLowestIndex()),
Register(extra_temps.PopLowestIndex()),
Register(extra_temps.PopLowestIndex()));
I did it this way because if there are only two registers explicitly
passed as temps, but we happen to have three scratch registers
available, we can still take advantage of the fastest helper. Arranging
it as you described is cleaner, I agree, but it hard-codes the
assumption that we have two masm scratch registers. That might be a
reasonable thing to assume, though.
Unless you say otherwise, I'll modify this as you described.
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode2506
src/a64/macro-assembler-a64.cc:2506: CopyFieldsUnrolledHelper(dst, src,
count,
It's also worth noting that since the merge, we always provide three
scratch registers to CopyFields, so CopyFieldsUnrolledHelper is now
unused, and we only need to test 'count >= kLoopThreshold' to determine
which helper to call. I'll fix that in a separate patch.
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode4148
src/a64/macro-assembler-a64.cc:4148: temps.Include(scratch1, scratch2);
On 2014/02/24 11:04:49, rmcilroy wrote:
nit - is this Include really required here?
Yes, I'm afraid so. JumpIfNotInNewSpace needs some scratch registers,
but in some contexts we've used them all up. It's a little irritating
that we have to do this, but I think this is the only case where it's
required.
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode5009
src/a64/macro-assembler-a64.cc:5009: exclude_all.ExcludeAll();
On 2014/02/24 11:04:49, rmcilroy wrote:
Can't we just simplify this to assert that arg0, arg1, etc are not
members of
the available tmplist, and just acquire a temp from that list when
required by
this function?
Perhaps, but this way allows us to call Printf on allocatable scratch
registers. That might not matter in any sensible usage, but Printf is
like a big hammer; it should always work when you're trying to debug
weird things, even if you wouldn't call it in that way in normal code.
As a side-note, the only registers than we cannot Printf are
StackPointer() and csp. (I have a plan for printing those too, as that
might be useful at times.)
Also, using the APIs that exist at the moment, it would take four
separate checks to assert that arg0-3 aren't in the TmpList():
ASSERT(arg0.IsNone() || !TmpList()->IncludesAliasOf(arg0));
...
We could change that, of course.
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode5172
src/a64/macro-assembler-a64.cc:5172: void
UseScratchRegisterScope::Include(const CPURegList& regs) {
On 2014/02/24 11:04:49, rmcilroy wrote:
Having had another look through the code, it seems that the Exclude
methods are
only used in PrintF, and the Include functions are only used in
MacroAssembler::CopyFields and MacroAssembler::RememberSetHelper
neither of
which I think requires the use of a UseScratchRegisterScope (see
comments on
these occurrences for details). I would really like it if we simplify
this
class to avoid having Include/Exclude methods and only enable scoping
of the
TempRegister lists. This would fit much better with the nice scope
based design
of the class in its intended use and avoid confusion and subtle bugs
if
registers were incorrectly included in the tmp list.
Would it be possible to remove the Include/Exclude methods or did you
have other
use-cases for Include/Exclude in the future?
They're useful in tests (such as Dump), but there aren't many cases
where we need to explicitly exclude registers. Most of the time, if we
need to use a specific register, it's because we're calling a function.
We _could_ go over-the-top and Exclude the arguments registers (x0, ...)
in those cases, to be safe, but such an invasive change is almost
certainly not worthwhile.
Regarding incorrect use: It is true that modifications to TmpList() will
be reverted when a scope exits, even if a caller started the scope, but
we thought that was the correct behaviour. What accidental use-case did
you think of?
Also, even if I remove Include and Exclude, the same behaviour could be
achieved by directly manipulating TmpList(), but I don't want to
encourage that.
https://codereview.chromium.org/164793003/
--
--
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.
