Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
164793003:
A64: Use a scope utility to allocate scratch registers. (Closed)
|
Created:
6 years, 10 months ago by jbramley Modified:
6 years, 9 months ago CC:
v8-dev Visibility:
Public. |
DescriptionA64: Use a scope utility to allocate scratch registers.
This replaces Tmp0() and Tmp1() with a more flexible scratch register
pool. A scope-based utility can temporarily acquire registers from this
pool as needed.
We no longer have to worry about whether to use Tmp0(), Tmp1() or
something else; the scope can just get the next available scratch
register.
BUG=
R=jochen@chromium.org, rmcilroy@chromium.org
Committed: https://code.google.com/p/v8/source/detail?r=19768
Patch Set 1 #
Total comments: 15
Patch Set 2 : Remove Include() and Release(). #
Total comments: 3
Patch Set 3 : Modify Printf (as discussed) and remove Exclude(). #
Total comments: 8
Patch Set 4 : Address the latest review comments. #Patch Set 5 : Address _all_ of the latest review comments. #
Total comments: 20
Patch Set 6 : Fix latest issues. #Patch Set 7 : Rebase. #
Messages
Total messages: 25 (0 generated)
Looks good overall to me, just one minor suggestion. 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.... src/a64/macro-assembler-a64.cc:5178: include &= ~(xzr.Bit() | csp.Bit()); Maybe add an assert here too to warn if someone tries to add them?
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.... src/a64/macro-assembler-a64.cc:5178: include &= ~(xzr.Bit() | csp.Bit()); On 2014/02/20 13:06:00, rmcilroy wrote: > Maybe add an assert here too to warn if someone tries to add them? My reasoning was that in several MacroAssembler operations, we can treat the result as a scratch register (as long as it's not an input): temps.Include(rd); temps.Exclude(rn, rm); However, rd can be csp in most cases. So as not to complicate the logic in these operations, I thought it best to just ignore them, so we can do temps.Include(rd) without having to check that it's not csp. Note that I haven't actually made that optimisation; at the moment it isn't necessary, and this patch is big enough already. Would you prefer that I put the assertion in now, and then remove it later if necessary?
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.... src/a64/macro-assembler-a64.cc:2335: Csel(output.W(), output.W(), 255, le); Could you pull out the behaviour change (changing gt to le etc.) into a separate CL please. https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:2489: masm_temps.Include(temps); I actually think the old approach here of popping explicitly from temps was cleaner and more understandable (and would enable removal of The Include method). You could replace the methods below with something like: CopyFieldsLoopPairsHelper(dst, src, count, masm_temps.AquireX(), masm_temps.AquireX(), Register(extra_temps.PopLowestIndex()), Register(extra_temps.PopLowestIndex()), Register(extra_temps.PopLowestIndex())); https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:4148: temps.Include(scratch1, scratch2); nit - is this Include really required here? https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:5009: exclude_all.ExcludeAll(); Can't we just simplify this to assert that arg0, arg1, etc are not members of the available tmplist, and just acquire a temp from that list when required by this function? https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:5172: void UseScratchRegisterScope::Include(const CPURegList& regs) { Having had another look through the code, it seems that the Exclude methods are only used in PrintF, and the Include functions are only used in MacroAssembler::CopyFields and MacroAssembler::RememberSetHelper neither of which I think requires the use of a UseScratchRegisterScope (see comments on these occurrences for details). I would really like it if we simplify this class to avoid having Include/Exclude methods and only enable scoping of the TempRegister lists. This would fit much better with the nice scope based design of the class in its intended use and avoid confusion and subtle bugs if registers were incorrectly included in the tmp list. Would it be possible to remove the Include/Exclude methods or did you have other use-cases for Include/Exclude in the future? https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:5178: include &= ~(xzr.Bit() | csp.Bit()); On 2014/02/20 13:23:12, jbramley wrote: > On 2014/02/20 13:06:00, rmcilroy wrote: > > Maybe add an assert here too to warn if someone tries to add them? > > My reasoning was that in several MacroAssembler operations, we can treat the > result as a scratch register (as long as it's not an input): > > temps.Include(rd); > temps.Exclude(rn, rm); > > However, rd can be csp in most cases. So as not to complicate the logic in these > operations, I thought it best to just ignore them, so we can do > temps.Include(rd) without having to check that it's not csp. > > Note that I haven't actually made that optimisation; at the moment it isn't > necessary, and this patch is big enough already. Would you prefer that I put the > assertion in now, and then remove it later if necessary? As outlined above, I would like it if this method could be removed entirely, but if that's not possible I would prefer that the assertion is added here now and removed later if required.
Thanks for the review! I realise that the patch is rather large. 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.... src/a64/macro-assembler-a64.cc:2335: Csel(output.W(), output.W(), 255, le); On 2014/02/24 11:04:49, rmcilroy wrote: > Could you pull out the behaviour change (changing gt to le etc.) into a separate > CL please. The behaviour is the same; I swapped the arguments because only the second operand can take an immediate. I had to invert the condition code accordingly. (When this code was written, Csel couldn't accept an Operand so we had to pass in a register.) https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:2489: masm_temps.Include(temps); On 2014/02/24 11:04:49, rmcilroy wrote: > I actually think the old approach here of popping explicitly from temps was > cleaner and more understandable (and would enable removal of The Include > method). You could replace the methods below with something like: > > CopyFieldsLoopPairsHelper(dst, src, count, > masm_temps.AquireX(), > masm_temps.AquireX(), > Register(extra_temps.PopLowestIndex()), > Register(extra_temps.PopLowestIndex()), > Register(extra_temps.PopLowestIndex())); I did it this way because if there are only two registers explicitly passed as temps, but we happen to have three scratch registers available, we can still take advantage of the fastest helper. Arranging it as you described is cleaner, I agree, but it hard-codes the assumption that we have two masm scratch registers. That might be a reasonable thing to assume, though. Unless you say otherwise, I'll modify this as you described. https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:2506: CopyFieldsUnrolledHelper(dst, src, count, It's also worth noting that since the merge, we always provide three scratch registers to CopyFields, so CopyFieldsUnrolledHelper is now unused, and we only need to test 'count >= kLoopThreshold' to determine which helper to call. I'll fix that in a separate patch. https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:4148: temps.Include(scratch1, scratch2); On 2014/02/24 11:04:49, rmcilroy wrote: > nit - is this Include really required here? Yes, I'm afraid so. JumpIfNotInNewSpace needs some scratch registers, but in some contexts we've used them all up. It's a little irritating that we have to do this, but I think this is the only case where it's required. https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:5009: exclude_all.ExcludeAll(); On 2014/02/24 11:04:49, rmcilroy wrote: > Can't we just simplify this to assert that arg0, arg1, etc are not members of > the available tmplist, and just acquire a temp from that list when required by > this function? Perhaps, but this way allows us to call Printf on allocatable scratch registers. That might not matter in any sensible usage, but Printf is like a big hammer; it should always work when you're trying to debug weird things, even if you wouldn't call it in that way in normal code. As a side-note, the only registers than we cannot Printf are StackPointer() and csp. (I have a plan for printing those too, as that might be useful at times.) Also, using the APIs that exist at the moment, it would take four separate checks to assert that arg0-3 aren't in the TmpList(): ASSERT(arg0.IsNone() || !TmpList()->IncludesAliasOf(arg0)); ... We could change that, of course. https://codereview.chromium.org/164793003/diff/1/src/a64/macro-assembler-a64.... src/a64/macro-assembler-a64.cc:5172: void UseScratchRegisterScope::Include(const CPURegList& regs) { On 2014/02/24 11:04:49, rmcilroy wrote: > Having had another look through the code, it seems that the Exclude methods are > only used in PrintF, and the Include functions are only used in > MacroAssembler::CopyFields and MacroAssembler::RememberSetHelper neither of > which I think requires the use of a UseScratchRegisterScope (see comments on > these occurrences for details). I would really like it if we simplify this > class to avoid having Include/Exclude methods and only enable scoping of the > TempRegister lists. This would fit much better with the nice scope based design > of the class in its intended use and avoid confusion and subtle bugs if > registers were incorrectly included in the tmp list. > > Would it be possible to remove the Include/Exclude methods or did you have other > use-cases for Include/Exclude in the future? 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? 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.
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.... 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. > 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 :).
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.... > 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.)
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.... > > 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.
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.... > > > 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.
Sorry - I didn't realize that you had submitted another patch set and was waiting for that. Please ping me in the future if I'm taking too long. One final comment in PrintF but otherwise looks good. https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.cc (right): https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4993: FPTmpList()->Combine(kCallerSavedFP); 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. Maybe you could just use: Register tmp = GetAllocatableRegisterThatIsNotOneOf(arg0..arg3)? Not sure if any of these other macroassembler calls could use tmp registers internally as well though?
https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.cc (right): https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4993: FPTmpList()->Combine(kCallerSavedFP); 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.
https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.cc (right): https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... 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.
On 2014/02/27 11:49:28, rmcilroy wrote: > https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... > File src/a64/macro-assembler-a64.cc (right): > > https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... > 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.
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... > > File src/a64/macro-assembler-a64.cc (right): > > > > > https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... > > 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 ;).
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... > > > File src/a64/macro-assembler-a64.cc (right): > > > > > > > > > https://codereview.chromium.org/164793003/diff/190001/src/a64/macro-assembler... > > > 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.
Looks much better to me, just a couple more small nits. One thing - could you please send a message when you upload a new patch set which you want me to review - I don't get a notification when you upload a patch set, just when you send me a message (and you sent you last message a few hours before uploading the patch-set). https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.cc (right): https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4828: ASSERT(!TmpList()->IncludesAliasOf(arg0)); nit - ASSERT(!(TmpList()->IncludesAliasOf(arg0) || FPTmpList->IncludesAliasOf(arg0))) etc. https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4984: RegList old_fptmp_list = FPTmpList()->list(); nit - old_fp_tmp_list (not old_fptmp_list) https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4996: CPURegList fptmp_list = kCallerSavedFP; nit - fp_tmp_list (not fptmp_list) https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.h (right): https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2232: static void ReleaseByCode(CPURegList* available, int code); I don't think you need ReleaseByCode, ReleaseByRegList or IncludeByRegList any more do you? Could you delete these please.
On 2014/02/28 13:44:30, rmcilroy wrote: > Looks much better to me, just a couple more small nits. > > One thing - could you please send a message when you upload a new patch set > which you want me to review - I don't get a notification when you upload a patch > set, just when you send me a message (and you sent you last message a few hours > before uploading the patch-set). Oh, strange, I assumed that it would send updates for patch sets and didn't want to spam you with a message as well. I'm still getting used to this review system. I'll upload a new patch shortly; I'll let you know when I do so.
The new patch set is ready. https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.cc (right): https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4828: ASSERT(!TmpList()->IncludesAliasOf(arg0)); On 2014/02/28 13:44:30, rmcilroy wrote: > nit - > ASSERT(!(TmpList()->IncludesAliasOf(arg0) || FPTmpList->IncludesAliasOf(arg0))) > etc. Fixed, though they won't fit on one line so I just made separate assertions. https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4984: RegList old_fptmp_list = FPTmpList()->list(); On 2014/02/28 13:44:30, rmcilroy wrote: > nit - old_fp_tmp_list (not old_fptmp_list) Done. https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4996: CPURegList fptmp_list = kCallerSavedFP; On 2014/02/28 13:44:30, rmcilroy wrote: > nit - fp_tmp_list (not fptmp_list) Done. https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.h (right): https://codereview.chromium.org/164793003/diff/250001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2232: static void ReleaseByCode(CPURegList* available, int code); On 2014/02/28 13:44:30, rmcilroy wrote: > I don't think you need ReleaseByCode, ReleaseByRegList or IncludeByRegList any > more do you? Could you delete these please. Ah yes, I missed those, thanks for the reminder!
Found a few more nits, but once these are addressed lgtm. https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.cc File src/a64/code-stubs-a64.cc (right): https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4718: UseScratchRegisterScope temps(masm); You don't need this anymore https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4782: UseScratchRegisterScope temps(masm); ditto https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4826: UseScratchRegisterScope temps(masm); ditto https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4860: UseScratchRegisterScope temps(masm); ditto https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.cc (right): https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4817: ASSERT(!TmpList()->IncludesAliasOf(arg0)); might be nice to have IncludesAliasOf be a multi-arg function (with default NoReg args) so you could just do: ASSERT(!TmpList()->IncludesAliasOf(arg0, arg1, arg2, arg3)); https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4829: ASSERT(!AreAliased(arg0, StackPointer())); nit - ASSERT(!AreAliased(StackPointer(), arg0, arg1, arg2, arg3)); (This would prevent passing multiple of the same reg as arguments, but I don't think we'd need that. If you disagree I'm fine leaving this as-is). https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.h (right): https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2212: remove extra newline (only two newlines between functions in cc files, not class definitions). https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2215: ditto https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2223: ditto https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2227: ditto
https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.cc File src/a64/code-stubs-a64.cc (right): https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4718: UseScratchRegisterScope temps(masm); On 2014/02/28 17:13:34, rmcilroy wrote: > You don't need this anymore Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4782: UseScratchRegisterScope temps(masm); On 2014/02/28 17:13:34, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4826: UseScratchRegisterScope temps(masm); On 2014/02/28 17:13:34, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/code-stubs-a64.... src/a64/code-stubs-a64.cc:4860: UseScratchRegisterScope temps(masm); On 2014/02/28 17:13:34, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.cc (right): https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4817: ASSERT(!TmpList()->IncludesAliasOf(arg0)); On 2014/02/28 17:13:34, rmcilroy wrote: > might be nice to have IncludesAliasOf be a multi-arg function (with default > NoReg args) so you could just do: > ASSERT(!TmpList()->IncludesAliasOf(arg0, arg1, arg2, arg3)); Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.cc:4829: ASSERT(!AreAliased(arg0, StackPointer())); On 2014/02/28 17:13:34, rmcilroy wrote: > nit - > ASSERT(!AreAliased(StackPointer(), arg0, arg1, arg2, arg3)); > (This would prevent passing multiple of the same reg as arguments, but I don't > think we'd need that. If you disagree I'm fine leaving this as-is). No, it's useful to pass the same register several times, for example so you can print it in several formats. Again, Printf is a hammer, and it should always work. https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... File src/a64/macro-assembler-a64.h (right): https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2212: On 2014/02/28 17:13:34, rmcilroy wrote: > remove extra newline (only two newlines between functions in cc files, not class > definitions). Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2215: On 2014/02/28 17:13:34, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2223: On 2014/02/28 17:13:34, rmcilroy wrote: > ditto Done. https://codereview.chromium.org/164793003/diff/290001/src/a64/macro-assembler... src/a64/macro-assembler-a64.h:2227: On 2014/02/28 17:13:34, rmcilroy wrote: > ditto Done.
lgtm
On 2014/03/03 10:57:40, rmcilroy wrote: > lgtm I need an lgtm from an owner of test/cctest/test-assembler-a64.cc and test/cctest/test-utils-a64.cc. Ulan and Jochen: Could either of you help?
lgtm
Message was sent while issue was closed.
Committed patchset #7 manually as r19768 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
