VirtRegMap.cpp should not just remove an identity copy which also has a impl-def of a super-reg

[jonpa]

Summary

VirtRegMap removes identity copies, but that leads to machine-verifier complaints if there is a def of a super reg as well.

I am not sure what the best fix is, but the patch attached seems to fix this by transforming the COPY to an IMPLICIT_DEF when there is a def of a super-reg. The patch assumes (arbitrarily) that there could only be one extra operand - not sure if this is always true...

I put this with a testcase on Bugzilla a while ago and would appreciate any review.
https://llvm.org/bugs/show_bug.cgi?id=27156

[qcolombet]

We could fix MachineInstr:: isIdentityCopy instead. Indeed, the next optimization that removes them will hit the same issue. Moreover, I do not think we are supposed to have implicit_def around after TwoAddrPass (not entirely sure about that though).

Q.

[qcolombet]

Also, side question, how do we end up with this representation?
Do we use an undef lane or something, that’s why the verifier complains?

lib/CodeGen/VirtRegMap.cpp

for (MachineBasicBlock::instr_iterator
         MI->addRegisterDefined(SuperDefs.pop_back_val(), TRI);
 
       DEBUG(dbgs() << "> " << *MI);
 
       // Finally, remove any identity copies.
       if (MI->isIdentityCopy()) {
         ++NumIdCopies;
         DEBUG(dbgs() << "Deleting identity copy.\n");
+
+        // A COPY which has an implicit def of a super register must
+        // become an IMPLICIT_DEF of that register.
+        if (MI->getNumOperands() > 2) {
+          MachineOperand &MO = MI->getOperand(2);
+          if (MO.isDef() && MO.isImplicit()) {
+            MI->setDesc(TII->get(TargetOpcode::IMPLICIT_DEF));
+            MI->RemoveOperand(0);
+            MI->RemoveOperand(0);
+            MI->getOperand(0).setImplicit(false);
+            assert (MI->getNumOperands() == 1 && "Not a single super reg implicit def?");
+          }
+        }
+        else {
           if (Indexes)
             Indexes->removeMachineInstrFromMaps(*MI);
           // It's safe to erase MI because MII has already been incremented.
           MI->eraseFromParent();
         }
       }
     }
   }
+}