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