[llvm-commits] [llvm] r56268 - in /llvm/trunk: include/llvm/CodeGen/LinkAllCodegenComponents.h include/llvm/CodeGen/Passes.h lib/CodeGen/DeadMachineInstructionElim.cpp

[Dan Gohman]

On Sep 21, 2008, at 9:24 PM, Chris Lattner wrote:

>
> On Sep 16, 2008, at 5:43 PM, Dan Gohman wrote:
>
>> +bool  
>> DeadMachineInstructionElim::runOnMachineFunction(MachineFunction  
>> &MF) {
>> +  bool AnyChanges = false;
>> +  const MachineRegisterInfo &MRI = MF.getRegInfo();
>> +  const TargetInstrInfo &TII = *MF.getTarget().getInstrInfo();
>> +  bool SawStore = true;
>> +
>> +  // Loop over all instructions in all blocks, from bottom to top,  
>> so that it's
>> +  // more likely that chains of dependent but ultimately dead  
>> instructions will
>> +  // be cleaned up.
>> +  for (MachineFunction::reverse_iterator I = MF.rbegin(), E =  
>> MF.rend();
>> +       I != E; ++I) {
>
> This looks pretty nice to me.  The one big "problem" is that defs of  
> physregs prevent an instruction from being removed.  Since physreg  
> defs can only be local before regalloc, and since you're walking  
> backward through the block already, can you just keep track of uses  
> of physregs as you scan backward?

Yes, this is the approach I ended up taking, at least for now.

>
>
>> +    MachineBasicBlock *MBB = &*I;
>> +    for (MachineBasicBlock::reverse_iterator MII = MBB->rbegin(),
>> +         MIE = MBB->rend(); MII != MIE; ) {
>> +      MachineInstr *MI = &*MII;
>> +
>> +      // Don't delete instructions with side effects.
>> +      if (MI->isSafeToMove(&TII, SawStore)) {
>
> Is this really the right predicate?  Why not check explicitly for  
> side effects?

There are several kinds of side effects: stores, volatile loads,
calls, branches, and so on, and isSafeToMove checks all of them.

>
>
> Just to avoid nesting, how about:
>
> if (!MI->isSafeToMove(&TII, SawStore)) {
>  ++MII;
>  continue;
> }

Done.

>
>
>>
>> +        // Examine each operand.
>> +        bool AllDefsDead = true;
>> +        for (unsigned i = 0, e = MI->getNumOperands(); i != e; + 
>> +i) {
>> +          const MachineOperand &MO = MI->getOperand(i);
>> +          if (MO.isRegister() && MO.isDef()) {
>> +            unsigned Reg = MO.getReg();
>> +            if (TargetRegisterInfo::isPhysicalRegister(Reg) ||
>> +                !MRI.use_empty(Reg)) {
>> +              // This def has a use. Don't delete the instruction!
>> +              AllDefsDead = false;
>> +              break;
>> +            }
>> +          }
>> +        }
>
> Please split this out to a helper function.

Done.

>
>
>>
>> +
>> +        // If there are no defs with uses, the instruction is dead.
>> +        if (AllDefsDead) {
>> +          // Clear out the operands to take the registers out of  
>> their
>> +          // use chains.
>> +          while (unsigned Num = MI->getNumOperands())
>> +            MI->RemoveOperand(Num-1);
>
> Why do you need to do this?  Just deleting the MI should be enough.


It was a misunderstanding on my part. I removed these lines now.

Dan