[llvm-commits] [llvm] r51562 - /llvm/trunk/lib/CodeGen/TwoAddressInstructionPass.cpp

[Bill Wendling]

On Tue, May 27, 2008 at 11:08 AM, Evan Cheng <evan.cheng at apple.com> wrote:
> On May 25, 2008, at 10:18 PM, Bill Wendling wrote:
>
>> --- llvm/trunk/lib/CodeGen/TwoAddressInstructionPass.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/TwoAddressInstructionPass.cpp Mon May 26
>> @@ -321,7 +324,14 @@
>>
>>         InstructionRearranged:
>>           const TargetRegisterClass* rc =
>> MF.getRegInfo().getRegClass(regA);
>> -          TII->copyRegToReg(*mbbi, mi, regA, regB, rc, rc);
>> +          MachineInstr *Orig = MRI->getVRegDef(regB);
>> +
>> +          if (Orig && TII->isTriviallyReMaterializable(Orig)) {
>> +            TII->reMaterialize(*mbbi, mi, regA, Orig);
>> +            ReMattedInstrs.insert(Orig);
>
> Do you see any performance impact from this patch? It's not clear this
> is always a good idea. It's almost always a good idea to remat instead
> of spill, but not necessarily the case here. I wonder if this needs to
> check if the instruction is "cheap"?
>
I ran the llvm-tests to check for correctness (though your PIC run had
some failures for the Beta that I set up). I didn't run it to check
for performance. I'll do that and see what's up.

>> @@ -357,5 +367,34 @@
>>     }
>>   }
>>
>> +  SmallPtrSet<MachineInstr*, 8>::iterator I = ReMattedInstrs.begin();
>> +  SmallPtrSet<MachineInstr*, 8>::iterator E = ReMattedInstrs.end();
>> +
>> +  for (; I != E; ++I) {
>> +    MachineInstr *MI = *I;
>> +    bool InstrDead = true;
>> +
>> +    for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
>> +      const MachineOperand &MO = MI->getOperand(i);
>> +      if (!MO.isRegister())
>> +        continue;
>> +      unsigned MOReg = MO.getReg();
>> +      if (!MOReg)
>> +        continue;
>> +      if (MO.isDef()) {
>> +        if (MO.isImplicit())
>> +          continue;
>
> How about?
>
>        is (!MOReg || !MO.isDef() || MO.isImplicit())
>          continue;
>
Sure. I'll combine it with your previous email and have it also check
(MO.isImplicit() && MO.isDead()).

>> +        if (MRI->use_begin(MOReg) != MRI->use_end()) {
>> +          InstrDead = false;
>> +          break;
>> +        }
>> +      }
>> +    }
>> +
>> +    if (InstrDead && MI->getNumOperands() > 0)
>
> Why check for MI->getNumOperands() > 0?
>
Paranoia :-)

-bw