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);
Could you pull out the behaviour change (changing gt to le etc.) into a
separate CL please.
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);
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()));
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);
nit - is this Include really required here?
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode5009
src/a64/macro-assembler-a64.cc:5009: exclude_all.ExcludeAll();
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?
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) {
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?
https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.cc#newcode5178
src/a64/macro-assembler-a64.cc:5178: include &= ~(xzr.Bit() |
csp.Bit());
On 2014/02/20 13:23:12, jbramley wrote:
On 2014/02/20 13:06:00, rmcilroy wrote:
> Maybe add an assert here too to warn if someone tries to add them?
My reasoning was that in several MacroAssembler operations, we can
treat the
result as a scratch register (as long as it's not an input):
temps.Include(rd);
temps.Exclude(rn, rm);
However, rd can be csp in most cases. So as not to complicate the
logic in these
operations, I thought it best to just ignore them, so we can do
temps.Include(rd) without having to check that it's not csp.
Note that I haven't actually made that optimisation; at the moment it
isn't
necessary, and this patch is big enough already. Would you prefer that
I put the
assertion in now, and then remove it later if necessary?
As outlined above, I would like it if this method could be removed
entirely, but if that's not possible I would prefer that the assertion
is added here now and removed later if required.
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.
