[TailDuplicator] Fix old bugs in TailDuplicator::duplicateInstruction

[bjope]

Summary

This patch is updating TailDuplicator::duplicateInstruction to fix
some old bugs that has been found with an out-of-tree target. There
are three different things being addressed:

1. In one situation two subregister indices are combined using the composeSubRegIndices helper. But the order in which those indices are combined has been incorrect. For this problem I managed to create some kind of reproducer using AArch64 (see the test case touched in this patch).

2. Another fault was found in the else branch for the above situation. Here we do not compose the two subregisters, instead we insert a COPY to replace the PHI, and then the subreg index in the using MO remains. Thus, the virtual register created for the COPY should always match with the size of the original register. Therefore the optimization that "constrain" (or rather relax) the register class by looking at the instruction desc must be limited to the situation when there is no subregister access. Otherwise we create a vreg with the wrong class.

3. Last problem addressed in this patch is that when a new register class is picked by looking at the instruction desc, then it isn't guaranteed that the isAllocatable property is set for that class. So one need to use the getAllocatableClass helper to find a subclass that is allocatable before using createVirualRegister, or alternatively (as in this patch) just use the OrigRC instead of relaxing the register class for the COPY destination.

Haven't been able to find any in-tree reproducers for problem 2 and 3.
The tricky part is to find a target that has register hierarchies that
match with the problem to trigger those code paths (and with subreg
accesses involved).

[bjope]

Gentle ping!

Let me know if I should try to find more reviewers, not sure who else is good at subreg hierarchies etc.
But afaict the erroneous code was added in this commit by @kparzysz :

commit 4773f647bdf86da3fb95d7e34513d12bea55d60d
Author: Krzysztof Parzyszek <[e-mail]>
Date:   Tue Apr 26 18:36:34 2016 +0000

    [Tail duplication] Handle source registers with subregisters
    
    When a block is tail-duplicated, the PHI nodes from that block are
    replaced with appropriate COPY instructions. When those PHI nodes
    contained use operands with subregisters, the subregisters were
    dropped from the COPY instructions, resulting in incorrect code.
    
    Keep track of the subregister information and use this information
    when remapping instructions from the duplicated block.
    
    Differential Revision: http://reviews.llvm.org/D19337
    
    llvm-svn: 267583

[kparzysz]

llvm/lib/CodeGen/TailDuplicator.cpp

444–445

Maybe we can just use OrigRC here, instead of trying to come up with a new class? OrigRC is guaranteed to be valid, since that's what the original code used.

[bjope]

llvm/lib/CodeGen/TailDuplicator.cpp

444–445

Yes. That is an alternative solution.

If I remember correct I tried to avoid changing the behavior for the non-faulty case (assuming that there was some good reason why the old code tried to find a new RC here).

Since register coalescing is doing register class widening (taking getLargestLegalSuperClass into consideration) I don't think that it is important to find a "better" class here. And there are no lit tests that are impacted if I simplify this to just use OrigRC (and I haven't seen any regressions in downstream testing either).

I'll update the patch accordingly, to simplify this part.

[bjope]

Simplified code based on review feedback.

[kparzysz]

LGTM, thanks!

llvm/lib/CodeGen/TailDuplicator.cpp

for (unsigned i = 0, e = NewMI.getNumOperands(); i != e; ++i) {
           }
 
           if (ConstrRC) {
             // If the class constraining succeeded, we can simply replace
             // the old register with the mapped one.
             MO.setReg(VI->second.Reg);
             // We have Reg -> VI.Reg:VI.SubReg, so if Reg is used with a
             // sub-register, we need to compose the sub-register indices.
-            MO.setSubReg(TRI->composeSubRegIndices(MO.getSubReg(),
-                                                   VI->second.SubReg));
+            MO.setSubReg(
+                TRI->composeSubRegIndices(VI->second.SubReg, MO.getSubReg()));
           } else {
             // The direct replacement is not possible, due to failing register
             // class constraints. An explicit COPY is necessary. Create one
-            // that can be reused
-            auto *NewRC = MI->getRegClassConstraint(i, TII, TRI);
-            if (NewRC == nullptr)
-              NewRC = OrigRC;
-            Register NewReg = MRI->createVirtualRegister(NewRC);
+            // that can be reused.
+            Register NewReg = MRI->createVirtualRegister(OrigRC);
             BuildMI(*PredBB, NewMI, NewMI.getDebugLoc(),
                     TII->get(TargetOpcode::COPY), NewReg)
                 .addReg(VI->second.Reg, 0, VI->second.SubReg);
             LocalVRMap.erase(VI);
             LocalVRMap.insert(std::make_pair(Reg, RegSubRegPair(NewReg, 0)));
             MO.setReg(NewReg);
             // The composed VI.Reg:VI.SubReg is replaced with NewReg, which
             // is equivalent to the whole register Reg. Hence, Reg:subreg

llvm/test/CodeGen/AArch64/taildup-subreg-compose.mir

 tracksRegLiveness: true
 body:             |
   ; CHECK-LABEL: name: foo2
   ; CHECK: bb.0.entry:
   ; CHECK-NEXT:   successors: %bb.4(0x80000000)
   ; CHECK-NEXT:   liveins: $q1
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   [[COPY:%[0-9]+]]:fpr128 = COPY $q1
-  ; CHECK-NEXT:   $s1 = COPY [[COPY]].dsub
+  ; CHECK-NEXT:   $s1 = COPY [[COPY]].ssub
   ; CHECK-NEXT:   [[COPY1:%[0-9]+]]:fpr64 = COPY [[COPY]].dsub
   ; CHECK-NEXT:   B %bb.4
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.1:
   ; CHECK-NEXT:   successors: %bb.4(0x80000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   [[DEF:%[0-9]+]]:fpr128 = IMPLICIT_DEF
-  ; CHECK-NEXT:   $s1 = COPY [[DEF]].dsub
+  ; CHECK-NEXT:   $s1 = COPY [[DEF]].ssub
   ; CHECK-NEXT:   [[COPY2:%[0-9]+]]:fpr64 = COPY [[DEF]].dsub
   ; CHECK-NEXT:   B %bb.4
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT: bb.3:
   ; CHECK-NEXT:   successors: %bb.3(0x80000000)
   ; CHECK-NEXT: {{  $}}
   ; CHECK-NEXT:   B %bb.3
   ; CHECK-NEXT: {{  $}}