On 2014/02/27 13:53:30, rmcilroy wrote:
On 2014/02/27 12:11:00, jbramley wrote:
> On 2014/02/27 11:49:28, rmcilroy wrote:
> >
>
https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler-a64.cc
> > File src/a64/macro-assembler-a64.cc (right):
> >
> >
>
https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler-a64.cc#newcode4993
> > src/a64/macro-assembler-a64.cc:4993:
FPTmpList()->Combine(kCallerSavedFP);
> > On 2014/02/27 11:27:16, jbramley wrote:
> > > On 2014/02/27 11:17:32, rmcilroy wrote:
> > > > Do we really need all these registers as temps? Seems quite
hacky to
> modify
> > > the
> > > > TmpList manually here and rely on the UseScratchRegisterScope to
restore
> it.
> > >
> > > No, it doesn't need them, but it's easier to provide them all than
to
select
> a
> > > couple from the list. We have to preserve all caller-saved
registers for
the
> > > call anyway, so they'll all potentially be clobbered.
> > >
> > > This should have been temps.Include() rather than a TmpList() hack,
and
I
> > would
> > > have changed it had I not removed Include().
> > >
> > > > Maybe you could just use:
> > > > Register tmp = GetAllocatableRegisterThatIsNotOneOf(arg0..arg3)?
> > >
> > > GetAllocatableRegisterThatIsNotOneOf doesn't work because the list
of
> > > allocatable scratch registers includes some callee-saved registers.
> > >
> > > > Not sure if any of these other macroassembler calls could use tmp
> registers
> > > > internally as well though?
> > >
> > > Theoretically they might, but in practice, I think the only one here
that
> uses
> > > scratch registers is PrintfNoPreserve.
> >
> > I still don't like this - it feels like a hack and a misuse of
> > UseScratchRegisterScope. I would prefer if you made it possible to
specify
an
> > allocatable set of registers to GetAllocatableRegisterThatIsNotOneOf,
and
then
> > use that passing in kCallerSaved. If it really isn't possible then I
suppose
> I
> > can live with it if it's only in PrintF, but I would this change.
>
> How many scratch registers do I request? I need one here, to preserve
NZCV.
I
> think PrintfNoPreserve needs two (indirectly), but I'm not certain of
that,
and
> it depends on the implementation of the MacroAssembler functions that it
uses.
> That's three calls to GetAllocatableRegisterThatIsNotOneOf, and it's
inflexible
> in that PrintfNoPreserve might be able to be more efficient with more.
> Alternatively, it's wasteful if PrintfNoPreserve only uses one.
>
> Would it be better if I prepared the list first, then passed it in?
>
> { CPURegList scratch = kCallerSaved;
> if (scratch.IncludesAliasOf(arg0)) scratch.Remove(arg0);
> if (scratch.IncludesAliasOf(arg1)) scratch.Remove(arg1);
> if (scratch.IncludesAliasOf(arg2)) scratch.Remove(arg2);
> if (scratch.IncludesAliasOf(arg3)) scratch.Remove(arg3);
> TmpList()->set_list(scratch.list());
>
> CPURegList fpscratch = kCallerSavedFP;
> if (fpscratch.IncludesAliasOf(arg0)) fpscratch.Remove(arg0);
> if (fpscratch.IncludesAliasOf(arg1)) fpscratch.Remove(arg1);
> if (fpscratch.IncludesAliasOf(arg2)) fpscratch.Remove(arg2);
> if (fpscratch.IncludesAliasOf(arg3)) fpscratch.Remove(arg3);
> FPTmpList()->set_list(fpscratch.list());
>
> ...
>
> TmpList()->set_list(0);
> FPTmpList()->set_list(0);
> }
>
> That avoids UseScratchRegisterScope at this level. It's very verbose,
but it
> might be clearer, and it doesn't mix TmpList() with
UseScratchRegisterScope.
Ok this might be clearer and avoiding mixing UseScratchRegisterScope with
TmpList. Maybe something if you add a method to CPURegList to remove a an
arbritrary set of CPURegsiters from the list, e.g.,
CPURegList::Remove(reg0,
reg1= NoReg, ....) which does the "if (IncludesAliasOf(reg0))
Remove(reg0);
Yep, I can do that. At the moment, CPURegList::Remove asserts that the type
of
the register is the same as the type of the list, but I can change that, and
make it take multiple parameters.
..." this would allow you to do:
CPURegList original_tmp_list = TmpList();
TmpList()->set_list(0);
/// preserve CallerSaved ....
CPURegList tmp_scratch_list = kCallerSaved;
tmp_scratch_list.Remove(arg0, arg1, arg2, arg3);
TmpList()->set_list(tmp_scratch_list);
/// Call PrintfNoPreserve ....
TmpList()->set_list(original_tmp_list);
I think this would allow you to also remove the Exclude / ExcludeAll
functions
from UseScratchRegisterScope wouldn't it?
It would. Dump() still uses it (in the tests) but that could be rewritten
in a
similar way.
Also, do you need to add any
fpscratch registers, for PrintfNoPreserve or could you get away with just
excluding them all for the duration of the PrintF call?
The only MacroAssembler function that uses a FP scratch is Fcmp, so Printf
doesn't technically need any at the moment, but I don't like making that
kind of
assumption.
Thanks for sticking at this, apologies for being a pain ;).
No worries! The tree has been closed for some time so it's not exactly
holding
me up.
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.
