PeepholeOptimizer: Do not form PHI with subreg operands

[MatzeB]

Summary

When replacing a PHI the PeepholeOptimizer currently takes the register
class of the register at the first operand. This however is not correct
if this argument has a subregister index.

As there is currently no API to query the register class resulting from
applying a subregister index to all registers in a class, we can only
abort in these cases and not perform the transformation.

This changes findNextSource() to require the end of all copy chains to
not use a subregister if there is any PHI in the chain. I had to rewrite
the overly complicated inner loop there to have a good place to insert
the new check.

This fixes https://llvm.org/PR33071 (aka rdar://32262041)

[MatzeB]

Rebased to ToT and removed a cleanup/refactoring change that sneaked in here by accident.

@bruno: Quentin claims you know this code/loop in this patch better than him, could you take a look?

[MatzeB]

ping

[MatzeB]

ping2

[bruno]

LGTM

lib/CodeGen/PeepholeOptimizer.cpp

while (!SrcToLook.empty() && PHICount < RewritePHILimit) {
     TargetInstrInfo::RegSubRegPair Pair = SrcToLook.pop_back_val();
     // As explained above, do not handle physical registers
     if (TargetRegisterInfo::isPhysicalRegister(Pair.Reg))
       return false;
 
     CurSrcPair = Pair;
     ValueTracker ValTracker(CurSrcPair.Reg, CurSrcPair.SubReg, *MRI,
                             !DisableAdvCopyOpt, TII);
-    ValueTrackerResult Res;
-    bool ShouldRewrite = false;
 
-    do {
-      // Follow the chain of copies until we reach the top of the use-def chain
-      // or find a more suitable source.
-      Res = ValTracker.getNextSource();
+    // Follow the chain of copies until we find a more suitable source, a phi
+    // or have to abort.
+    while (true) {
+      ValueTrackerResult Res = ValTracker.getNextSource();
+      // Abort at the end of a chain (without finding a suitable source).
       if (!Res.isValid())
-        break;
+        return false;
 
       // Insert the Def -> Use entry for the recently found source.
       ValueTrackerResult CurSrcRes = RewriteMap.lookup(CurSrcPair);
       if (CurSrcRes.isValid()) {
         assert(CurSrcRes == Res && "ValueTrackerResult found must match");
         // An existent entry with multiple sources is a PHI cycle we must avoid.
         // Otherwise it's an entry with a valid next source we already found.
         if (CurSrcRes.getNumSources() > 1) {
while (true) {
       CurSrcPair.SubReg = Res.getSrcSubReg(0);
       // Do not extend the live-ranges of physical registers as they add
       // constraints to the register allocator. Moreover, if we want to extend
       // the live-range of a physical register, unlike SSA virtual register,
       // we will have to check that they aren't redefine before the related use.
       if (TargetRegisterInfo::isPhysicalRegister(CurSrcPair.Reg))
         return false;
 
+      // Keep following the chain if the value isn't any better yet.
       const TargetRegisterClass *SrcRC = MRI->getRegClass(CurSrcPair.Reg);
-      ShouldRewrite = TRI->shouldRewriteCopySrc(DefRC, SubReg, SrcRC,
-                                                CurSrcPair.SubReg);
-    } while (!ShouldRewrite);
+      if (!TRI->shouldRewriteCopySrc(DefRC, SubReg, SrcRC, CurSrcPair.SubReg))
+        continue;
 
-    // Continue looking for new sources...
-    if (Res.isValid())
+      // We currently cannot deal with subreg operands on PHI instructions
+      // (see insertPHI()).
+      if (PHICount > 0 && CurSrcPair.SubReg != 0)
         continue;
 
-    // Do not continue searching for a new source if the there's at least
-    // one use-def which cannot be rewritten.
-    if (!ShouldRewrite)
-      return false;
+      // We found a suitable source, and are done with this chain.
+      break;
     }
-
-  if (PHICount >= RewritePHILimit) {
-    DEBUG(dbgs() << "findNextSource: PHI limit reached\n");
-    return false;
   }
 
   // If we did not find a more suitable source, there is nothing to optimize.
   return CurSrcPair.Reg != Reg;
 }
 
 /// \brief Insert a PHI instruction with incoming edges \p SrcRegs that are
 /// guaranteed to have the same register class. This is necessary whenever we
 /// successfully traverse a PHI instruction and find suitable sources coming
 /// from its edges. By inserting a new PHI, we provide a rewritten PHI def
 /// suitable to be used in a new COPY instruction.
 static MachineInstr *
 insertPHI(MachineRegisterInfo *MRI, const TargetInstrInfo *TII,
           const SmallVectorImpl<TargetInstrInfo::RegSubRegPair> &SrcRegs,
           MachineInstr *OrigPHI) {
   assert(!SrcRegs.empty() && "No sources to create a PHI instruction?");
 
   const TargetRegisterClass *NewRC = MRI->getRegClass(SrcRegs[0].Reg);
+  // NewRC is only correct if no subregisters are involved. findNextSource()
+  // should have rejected those cases already.
+  assert(SrcRegs[0].SubReg == 0 && "should not have subreg operand");
   unsigned NewVR = MRI->createVirtualRegister(NewRC);
   MachineBasicBlock *MBB = OrigPHI->getParent();
   MachineInstrBuilder MIB = BuildMI(*MBB, OrigPHI, OrigPHI->getDebugLoc(),
                                     TII->get(TargetOpcode::PHI), NewVR);
 
   unsigned MBBOpIdx = 2;
   for (auto RegPair : SrcRegs) {
     MIB.addReg(RegPair.Reg, 0, RegPair.SubReg);

test/CodeGen/ARM/peephole-phi.mir

+# RUN: llc -o - %s -mtriple=armv7-- -verify-machineinstrs -run-pass=peephole-opt | FileCheck %s
+#
+# Make sure we do not crash on this input.
+# Note that this input could in principle be optimized, but right now we don't
+# have this case implemented so the output should simply be unchanged.
+#
+# CHECK-LABEL: name: func
+# CHECK: body: |
+# CHECK:   bb.0:
+# CHECK:     Bcc %bb.2, 1, undef %cpsr
+#
+# CHECK:   bb.1:
+# CHECK:     %0:dpr = IMPLICIT_DEF
+# CHECK:     %1:gpr, %2:gpr = VMOVRRD %0, 14, %noreg
+# CHECK:     B %bb.3
+#
+# CHECK:   bb.2:
+# CHECK:     %3:spr = IMPLICIT_DEF
+# CHECK:     %4:gpr = VMOVRS %3, 14, %noreg
+#
+# CHECK:   bb.3:
+# CHECK:     %5:gpr = PHI %1, %bb.1, %4, %bb.2
+# CHECK:     %6:spr = VMOVSR %5, 14, %noreg
+---
+name: func0
+tracksRegLiveness: true
+body: |
+  bb.0:
+    Bcc %bb.2, 1, undef %cpsr
+
+  bb.1:
+    %0:dpr = IMPLICIT_DEF
+    %1:gpr, %2:gpr = VMOVRRD %0:dpr, 14, %noreg
+    B %bb.3
+
+  bb.2:
+    %3:spr = IMPLICIT_DEF
+    %4:gpr = VMOVRS %3:spr, 14, %noreg
+
+  bb.3:
+    %5:gpr = PHI %1, %bb.1, %4, %bb.2
+    %6:spr = VMOVSR %5, 14, %noreg
+...
+
+# CHECK-LABEL: name: func1
+# CHECK:    %6:spr = PHI %0, %bb.1, %2, %bb.2
+# CHEKC:    %7:spr = COPY %6
+---
+name: func1
+tracksRegLiveness: true
+body: |
+  bb.0:
+    Bcc %bb.2, 1, undef %cpsr
+
+  bb.1:
+    %1:spr = IMPLICIT_DEF
+    %0:gpr = VMOVRS %1, 14, %noreg
+    B %bb.3
+
+  bb.2:
+    %3:spr = IMPLICIT_DEF
+    %2:gpr = VMOVRS %3:spr, 14, %noreg
+
+  bb.3:
+    %4:gpr = PHI %0, %bb.1, %2, %bb.2
+    %5:spr = VMOVSR %4, 14, %noreg
+...