Do not rename a tied operand in AggressiveAntiDepBreaker

[bcahoon]

Summary

The anti-dependence breaking code should not attempt to rename an operand that is tied. Otherwise, the two operand will end up with different registers. It appears that a similar check is made in CriticalAntiDepBreaker.

This is a failure I was seeing in the Hexagon. Unfortunately, I don't have a test case that fails with the in-tree Hexagon code. The failure in the machine verifier with the message "Two-address instruction operands must be identical" from the following instruction.

• instruction: %R21<def,tied1> = M2_acci
• operand 1: %R17<kill,tied0>

Prior to the anti dependence breaker (D10 is equivalent to R20 and R21).

%D10<def> = L4_loadrd_abs <ga:@Y+24>; 
...
%R21<def,tied1> = M2_acci %R21<kill,tied0>, %R2<kill>, %R20<kill>, %D10<imp-def>

And, then after (D8 is equivalent to R16 and R17)

%D8<def> = L4_loadrd_abs <ga:@Y+24>
,,,,
%R21<def,tied1> = M2_acci %R17<kill,tied0>, %R2<kill>, %R16<kill>, %D10<imp-def>

[hfinkel]

The anti-dependence breaking code should not attempt to rename an operand that is tied. Otherwise, the two operand will end up with different registers.

This does not seem quite true. It should either not rename them or rename them together. AggressiveAntiDepBreaker has logic which should handle tied operands. See AggressiveAntiDepBreaker::GetPassthruRegs and its various callers. If this logic is broken, then we should fix it.

[bcahoon]

Hi Hal - thanks for the review and your comments. I've looked into this issue some more, and I think that the problem (or part of the problem) is that Hexagon code adds a kill flag to the register use of the tied operand. For example,

%R21<def,tied1> = M2_acci %R21<kill,tied0>, %R2<kill>, %R20<kill>, %D10<imp-def>

I see in the target independent code, addRegisterKilled in MachineInstr.cpp, that there is code that explicitly checks for this case and will not add a kill flag to a tied operand that is a physical register. That code is,

if (isPhysReg && isRegTiedToDefOperand(i))
  // Two-address uses of physregs must not be marked kill.                                                        
  return true;
MO.setIsKill();

Once I fixed the Hexagon code so that we do not set the kill flag in this case, the bug in my test case no longer occurs.

I'm going to abandon this patch and fix the code that sets the kill flag in the Hexagon specific code.

lib/CodeGen/AggressiveAntiDepBreaker.cpp

for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
     DEBUG(dbgs() << " " << TRI->getName(Reg) << "=g" << State->GetGroup(Reg));
 
     // If MI's defs have a special allocation requirement, don't allow
     // any def registers to be changed. Also assume all registers
     // defined in a call must not be changed (ABI). Inline assembly may
     // reference either system calls or the register directly. Skip it until we
     // can tell user specified registers from compiler-specified.
     if (MI.isCall() || MI.hasExtraDefRegAllocReq() || TII->isPredicated(MI) ||
-        MI.isInlineAsm()) {
+        MI.isInlineAsm() || MI.isRegTiedToUseOperand(i)) {
       DEBUG(if (State->GetGroup(Reg) != 0) dbgs() << "->g0(alloc-req)");
       State->UnionGroups(Reg, 0);
     }
 
     // Any aliased that are live at this point are completely or
     // partially defined here, so group those aliases with Reg.
     for (MCRegAliasIterator AI(Reg, TRI, false); AI.isValid(); ++AI) {
       unsigned AliasReg = *AI;
for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
     DEBUG(dbgs() << " " << TRI->getName(Reg) << "=g" <<
           State->GetGroup(Reg));
 
     // It wasn't previously live but now it is, this is a kill. Forget
     // the previous live-range information and start a new live-range
     // for the register.
     HandleLastUse(Reg, Count, "(last-use)");
 
-    if (Special) {
+    if (Special || MI.isRegTiedToDefOperand(i)) {
       DEBUG(if (State->GetGroup(Reg) != 0) dbgs() << "->g0(alloc-req)");
       State->UnionGroups(Reg, 0);
     }
 
     // Note register reference...
     const TargetRegisterClass *RC = nullptr;
     if (i < MI.getDesc().getNumOperands())
       RC = TII->getRegClass(MI.getDesc(), i, TRI, MF);