[PATCH] D140496: [TailDuplicator] Fix old bugs in TailDuplicator::duplicateInstruction

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.
bjope added reviewers: kparzysz, arsenm.
Herald added subscribers: hiraditya, kristof.beyls.
Herald added a project: All.
bjope requested review of this revision.
Herald added a subscriber: wdng.
Herald added a project: LLVM.

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 is decided 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.

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).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140496

Files:
  llvm/lib/CodeGen/TailDuplicator.cpp
  llvm/test/CodeGen/AArch64/taildup-subreg-compose.mir


Index: llvm/test/CodeGen/AArch64/taildup-subreg-compose.mir
===================================================================
--- llvm/test/CodeGen/AArch64/taildup-subreg-compose.mir
+++ llvm/test/CodeGen/AArch64/taildup-subreg-compose.mir
@@ -23,7 +23,7 @@
   ; 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: {{  $}}
@@ -31,7 +31,7 @@
   ; 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: {{  $}}
Index: llvm/lib/CodeGen/TailDuplicator.cpp
===================================================================
--- llvm/lib/CodeGen/TailDuplicator.cpp
+++ llvm/lib/CodeGen/TailDuplicator.cpp
@@ -434,13 +434,21 @@
             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);
+            // that can be reused.
+            const TargetRegisterClass *NewRC = nullptr;
+            if (MO.getSubReg() == 0) {
+              // Try to get a larger register class (may improve coalescing?).
+              // We can't do this when a sub-register index is used in the
+              // MO. At least not if we want to update the LocalVRMap as the
+              // registers paired in the map must have the same size.
+              NewRC = TRI->getAllocatableClass(
+                  MI->getRegClassConstraint(i, TII, TRI));
+            }
             if (NewRC == nullptr)
               NewRC = OrigRC;
             Register NewReg = MRI->createVirtualRegister(NewRC);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D140496.484659.patch
Type: text/x-patch
Size: 2548 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221221/2fe84e80/attachment.bin>