PostRA Machine Sink should take care of COPY defining register that is a sub-register by another COPY source operand

[alex-t]

Summary

This pass was created with the assumption that all the copy sequences like

x = COPY y
z = COPY x

were already handled by the coalescer and transformed to

z = COPY y

This was true until the sub-registers appeared.

Considering the following pattern:

x = COPY y
z = COPY s
w = COPY x_z

where x_z is a super-reg consisting of x and y

Current implementation does not take into account registers defined by another copies assuming that one COPY cannot define register that is used by another one. As a result in sinks the COPY defining sub-register to one successor but COPY that reads super-register to another providing this later with uninitialized register.
Proposed fix updates lane mask after sinking the COPY rather then merely add whole COPY source to live ins.

[lebedev.ri]

Is there a test missing?

[nhaehnle]

With this change, it looks as though accumulateUsedDefed is called multiple times on some paths. That seems wrong. It does seem plausible that we'd have to keep accumulating for all code paths.

Also, this needs a test.

[alex-t]

In D71132#1774315, @nhaehnle wrote:

With this change, it looks as though accumulateUsedDefed is called multiple times on some paths. That seems wrong.

Yes. In fact this fix is wrong. Will be looking for another way to handle this.

[alex-t]

Just correct solution + tests

[vpykhtin]

Almost LGTM. Do you need those liveins reorderings?

llvm/test/CodeGen/AArch64/bisect-post-ra-machine-sink.mir

12 ↗

(On Diff #233443)

is this really needed?

[alex-t]

llvm/test/CodeGen/AArch64/bisect-post-ra-machine-sink.mir

12 ↗

(On Diff #233443)

sortUniqueLiveIns does sort. Hence we're going to have w0 before w1

[vpykhtin]

LGTM.

[arsenm]

llvm/lib/CodeGen/MachineSink.cpp

1286

Should use MCRegUnitMaskIterator instead of getting the lane masks for each subreg

[alex-t]

Updated according to reviewer request

llvm/lib/CodeGen/MachineSink.cpp

static void updateLiveIn(MachineInstr *MI, MachineBasicBlock *SuccBB,
   for (unsigned DefReg : DefedRegsInCopy)
     for (MCSubRegIterator S(DefReg, TRI, true); S.isValid(); ++S)
       SuccBB->removeLiveIn(*S);
   for (auto U : UsedOpsInCopy) {
     Register Reg = MI->getOperand(U).getReg();
     if (!SuccBB->isLiveIn(Reg))
       SuccBB->addLiveIn(Reg);
   }
 }
 
 static bool hasRegisterDependency(MachineInstr *MI,
                                   SmallVectorImpl<unsigned> &UsedOpsInCopy,
                                   SmallVectorImpl<unsigned> &DefedRegsInCopy,
                                   LiveRegUnits &ModifiedRegUnits,
                                   LiveRegUnits &UsedRegUnits) {
   bool HasRegDependency = false;
   for (unsigned i = 0, e = MI->getNumOperands(); i != e; ++i) {
for (auto I = CurBB.rbegin(), E = CurBB.rend(); I != E;) {
 
     if (MI->isDebugInstr())
       continue;
 
     // Do not move any instruction across function call.
     if (MI->isCall())
       return false;
 
-    if (!MI->isCopy() || !MI->getOperand(0).isRenamable()) {
-      LiveRegUnits::accumulateUsedDefed(*MI, ModifiedRegUnits, UsedRegUnits,
-                                        TRI);
+
+    if (!MI->isCopy() || !MI->getOperand(0).isRenamable()) {
       continue;
     }
 
     // Don't sink the COPY if it would violate a register dependency.
     if (hasRegisterDependency(MI, UsedOpsInCopy, DefedRegsInCopy,
                               ModifiedRegUnits, UsedRegUnits)) {
       LiveRegUnits::accumulateUsedDefed(*MI, ModifiedRegUnits, UsedRegUnits,
                                         TRI);