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);
..." 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? 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?
Thanks for sticking at this, apologies for being a pain ;).
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.
