[llvm-commits] [llvm] r80654 - in /llvm/trunk: lib/CodeGen/TwoAddressInstructionPass.cpp test/CodeGen/ARM/2009-08-31-TwoRegShuffle.ll

[Bob Wilson]

On Sep 1, 2009, at 12:37 AM, Evan Cheng wrote:

> Hi Bob,
>
> I am taking issues with this patch. The two-address pass is starting  
> to get too complicated (my fault).

No arguments about the complexity.  I made a few minor simplifications  
yesterday, but I'm sure there are more substantial improvements that  
could be made.

> I've been thinking about restructuring the code. This patch is  
> making the code even harder to read.

I tried pretty hard to simplify it.  This was the best I could come up  
with.  Maybe you understand more of the context and can suggest ways  
to simplify it further.

> It also potentially calls removeVirtualRegisterKilled() multiple  
> times for each source register.

I don't think that can happen.  It calls removeVirtualRegisterKilled  
when: KillMO is set and FirstKeptMO is false, i.e., the register is  
killed in this instruction, and none of the uses are tied to other  
destination registers.  Under those circumstances, all the uses of the  
register will be replaced.  Subsequent iterations cannot call  
removeVirtualRegisterKilled for the same register because all  
references to that register will be gone.

> Each of the call ends up scanning the entire operand list. That  
> seems inefficient to me.

See comments below.

> Also, what happens to the code the calls LV- 
> >removeVirtualRegisterDead(regB, mi)?

Sorry, I should have removed that in a separate patch.  We're  
modifying register uses here.  Uses have kill markers.  Defs have  
death markers.  If we don't change the defs, why would we need to call  
removeVirtualRegisterDead?  Am I missing something here?  I searched  
back through the svn history to see if this was added to fix a problem  
but it seems to go all the way back to the origins of this code.  I  
concluded that it is not needed, but I should have submitted this  
change separately.

>
>        if (KillMO) {
>          if (!FirstKeptMO) {
> 	    // All uses of regB are being replaced; move the kill to prevMI.
>            if (LV && LV->removeVirtualRegisterKilled(regB, mi))
>
> When KillMO is not null, it shouldn't be necessary to call  
> removeVirtualRegisterKilled, right?

KillMO is the machine operand that currently has a kill marker on it.   
If all of the register uses are being replaced, we want to move the  
kill to the preceding copy instruction.  This is no different than the  
previous code.

>
> Also notice we are scanning the operands twice:
>
>        for (unsigned i = 0, e = mi->getNumOperands(); i != e; ++i) {
>        ...
>        }
>
>        // Replace uses of regB with regA.
>        for (unsigned i = 0, e = mi->getNumOperands(); i != e; ++i) {
>        ...
>        }
>
> Please scan the operands just once. If it sees any other source  
> operand of the same virtual register tied to another def, then it  
> should not update the other operands of the same virtual register.  
> The next iteration will deal with them.

The first scan collects information.  The second modifies the  
operands. I first tried to do it in one scan but I could not come up  
with a reasonable way to make that work.  Note that  
removeVirtualRegisterKilled needs to be called before you update the  
registers, and since I didn't want to leave the instruction with  
incorrect kill information in between iterations, I needed to first  
check whether all the uses of the register should be replaced.

> As for the kill marker, it should move it to the previous  
> instruction the first time it is processed. All you have to ensure  
> is the later iteration does not insert any instruction between the  
> kill marker and the mi being processed. That should be pretty  
> straight forward. Just add a iterator which is the instruction  
> insertion location. The first time you insert a copy, it should then  
> be moved to the copy instruction, etc. e.g.
>
> a, b = op c<kill>, c   // insert pos
>
> =>
>
> a = c<kill>    // insert pos
> a, b = op a, c
>
> =>
>
> b = c
> a = c<kill>
> a, b = op a, b

That seems unsafe to me.  You're leaving the instruction in an  
inconsistent state.  After the first copy is inserted, you've moved  
the kill marker but there is still a use of c after the point where it  
is killed.  There is a lot of code inside this loop that is looking at  
register uses and kill markers.  On the next iteration, it seems like  
things may get confused.  It may be safe but I'm not able to easily  
convince myself of that, and even if it is safe now, it seems likely  
to be fragile in the future.

This seems like the root of your objections.  I am trying to keep the  
kill information up to date in between iterations.  Given that goal, I  
don't see a better way to write the code.  If you are certain that it  
is OK to leave the kill information in an incorrect state, I can  
change it as you suggest.  I would rather be safe here.

>
> Since you are working on this code, could be please clean it up a  
> bit. Can you factor this if statement:
>          if (mi->getOperand(ti).isDead() &&
>              isSafeToDelete(mi, regB, TII, Kills)) {
>
> to a separate function?

Like this (in the attached patch)?

Checking mi->getOperand(ti).isDead() is completely redundant with  
isSafeToDelete() so I assume it is only there to guard against calling  
a relatively expensive function in the common case.  If we're going to  
call a function anyway, maybe it would be better to just remove the  
isDead() check altogether.

I'd leave it as-is but I don't mind changing it if you tell me your  
preference.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: safeToDelete.patch
Type: application/octet-stream
Size: 1370 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090901/a52560cf/attachment.obj>