On 2014/02/24 14:29:47, rmcilroy wrote:
On 2014/02/24 13:53:13, jbramley wrote:
> On 2014/02/24 13:12:48, rmcilroy wrote:
> >
>
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#newcode5172
> > src/a64/macro-assembler-a64.cc:5172: void
> UseScratchRegisterScope::Include(const
> > CPURegList& regs) {
> >
> > > 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?
> >
> > The accidental use-case I'm thinking of is something along the lines
of
code
> > which adds a random register to a scratch register scope, does some
work,
then
> > accidentally uses that same register thinking it's free (either
within the
> same
> > scope, or a further scope of a called MacroAssembler method). E.g.:
> >
> > MacroAssembler::bar(... Register scratch) {
> > // load scratch1
> > Mov(x1, MemOperand...) // clobbers scratch1
> > // use scratch1
> > }
> >
> > MacroAssembler::foo() {
> > UseScratchRegisterScope tmps(this);
> > tmps.Include(scratch1);
> > // ...
> > bar(..., scratch1); // mistakenly think scratch1 is free
> > }
> >
> > I realise the use of scratch1 after including it in the
> UseScratchRegisterScope
> > in foo is incorrect, but I could see something like this easily being
> > accidentally introduced via copy/pasted code or in extra long scopes
where
> it's
> > not obvious which temps are available.
>
> Ok, I understand your reasoning.
>
> > > 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.
> >
> > Sure anything's possible by manipulating TmpList, but as you say we
don't
want
> > to encourage this, and Include/Exclude encourage this manipulation
of the
> > available scratch registers :).
>
> Fair enough, though I've hit a few problems removing them. I can work
around
not
> having Include in RememberedSetHelper by just passing in a single
scratch,
and
> calling AcquireX after the debug code. However, I can't do the same in
Printf
> without hacking TmpList() directly. (Actually I already did that, and I
> shouldn't have done.) Similarly, some of the tests are going to become
awkward,
> though that may be a lesser concern.
>
> Removing Include is quite easy (if a bit awkward in the tests), but
removing
> Exclude is hard. Exclude also seems somewhat safer (given the risk you
> described) in that it can only make registers unavailable. What do you
think
of
> just removing Include? I could also remove Release; I don't think we
use it
at
> all. (I wrote this mechanism for VIXL before porting it to V8, so these
things
> were provided as APIs even if VIXL itself doesn't use all of them.)
Ok this sounds a reasonable approach and I think would be safer. Could
you
get
away with just an ExcludeAll method, and not the other Exclude methods?
Thanks
for doing this!
Not really, I'm afraid. Again, Printf is the main culprit, but I expect that
there will be others too.
One final thought (feel free to ignore this as it would involve a fair
amount
of
work to rework things like this) - I wonder if it would be cleaner just to
have
a ScopedTempRegister type, which when allocated pops a tmp off of the
internal
assembler TmpList, and when destructed would just add the register back
into
TmpList? It could be a subclass of the appropriate register type and so
could
be used just like a register. This would mean instead of writing:
{ UseScratchRegisterScope tmps(this);
Register scratch = tmps.AquireX();
Mov(scratch1, x1); ...
}
You could just write:
{ ScopedTempRegister scratch1(this);
Mov(scratch1, x1); ...
}
You could also have a ExcludeTempRegistersScope object which did the same
as
the
UseScratchRegisterScope::ExcludeAll. What do you think, this might be
cleaner
if it would work? Either way I'm fine with UseScratchRegisterScope being
added,
even if it we decide that ScopedTempRegister might be a better long-term
approach.
I considered that approach, and I'd be quite happy to change later, though
I'd
rather not re-write it all now! We'd need variants for X, W, S and D
registers,
since the size can't be inferred. We'd also have to be careful about how the
destructor is implemented (since the order of execution of destructors is
not
defined), but it would be quite possible.
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.
