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

[Evan Cheng]

On Sep 1, 2009, at 10:50 AM, Bob Wilson wrote:

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

Ok. The logic is now getting fairly convoluted. I think it's time to  
consider overhauling it.

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

Ok.

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

Since you have already found the KillMO, there is no need to call  
removeVirtualRegisterKilled() which would scan the operands again.  
Just update LV varinfo and unset the kill marker.

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

I'll describe how I think the code should be restructured in a bit.

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

Ok. Good point. Let's consider a more extensive instead. How about  
something like this:

1. Scan an instruction. Look at all the operands. Keep a list of  
source registers that are tied to one or more def registers.
2. For each source register, track which def registers it is being  
tied to. It should also track the kill operand index.
3. Consider each source register. If it is only tied to one def  
register, we can consider performing the usual optimizations. If it is  
tied to multiple def, I don't think the current optimizations are  
useful.
4. If it is tied to multiple def, process it separately. For each of  
the tied def, add a copy (or remat the def). Move the kill marker to  
the last copy.

This should make the code more understandable.

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

Sorry, I meant factoring out lines 827 to 880 to a separate function.

Evan

>
> 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.
>
> <safeToDelete.patch>