[llvm-commits] [PATCH] Improve vmov domain conversion (followup)

[James Molloy]

Hi Jakob,

Thanks for the comments - it's taken me a day or two to get it rewritten
generically enough :)

The attached patch takes on board all of your comments: it adds an
"isRegisterLive" query function to MachineBasicBlock, and adds an
"analyzePhysReg" function to MachineInstr.

They seemed generic enough to lift into the appropriate classes.

How does this look to you?

Cheers,

James


On Tue, 2012-09-04 at 18:02 +0100, Jakob Stoklund Olesen wrote:
> On Sep 4, 2012, at 1:25 AM, James Molloy <james.molloy at arm.com> wrote:
> 
> > It defines a generic "isRegisterLive()" function that takes a
> > MachineInstr and register and will check if the register is live at
> > arrival at the MachineInstr.
> > 
> > It is O(N). :( . I couldn't see a better way of doing it though.
> > 
> > Could you please review?
> 
> Hi James,
> 
> Thanks for working on this!
> 
> The general approach is fine, but as I told Tim, you should limit the scan to a neighbourhood of MI to avoid the O(N) behaviour on large basic blocks. I would suggest looking at the 10 instructions before and the 10 instructions after MI. We can negotiate a higher limit, if you need it ;-)
> 
> Also note that when dealing with overlapping registers, liveness isn't a binary property. Your function could return one of:
> 
> - Reg is live.
> - Reg is not live, but some overlapping register is live.
> - Reg is not live, and no overlapping registers are live. (Thus, it is safe to clobber Reg).
> - Couldn't tell from local neighbourhood.
> 
> You could formulate this as answering two questions:
> 
> 1. Is Reg definitely live?
> 2. Is it definitely safe to clobber Reg?
> 
> Usually, code is looking for the answer to one of those questions. In the vmov case, it seems you need both.
> 
> 
> Regarding your patch, please don't use reverse_iterator. It is a quite confusing class that should only be used when necessary. (Stepping a reverse_iterator backwards is a double negative).
> 
> We've seen other code recently that calls MI->definesRegister() and MI->killsRegister() on the same instruction. I think it would make sense to add a MachineOperandIteratorBase::analyzePhysReg(Reg) function that scans the instruction operands once, and returns these flags:
> 
> Clobbers - Reg or an overlapping register is defined, or a regmask clobbers Reg. (Same as MI->clobbersPhysReg()).
> Defines - Reg or a super-register is defined. (Same as MI->definesRegister()).
> Reads - An operand reads Reg or a super-register. (As determined by readsReg()).
> ReadsOverlap - An operand reads Reg or an overlapping register.
> DefinesDead - All defs of Reg or a super-register are dead.
> Kills - There is a kill of Reg or a super-register. (Same as MI->killsRegister()).
> 
> /jakob
> 
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: improve-vmov-domain-conv.diff
Type: text/x-patch
Size: 13082 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120906/b958cd5c/attachment.bin>