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!
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.
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.
