[ARM] Be Conservative when coalescing into SP

[zzheng]

Summary

r226200 allows coalescing into SP but missed a corner case:
    
  %vreg907<def> = COPY %SP; GPRnopc:%vreg907
  %vreg908<def> = t2SUBri %vreg907, 80, pred:14, pred:%noreg, opt:%noreg; GPRnopc:%vreg908,%vreg907
  %SP<def> = COPY %vreg908; GPRnopc:%vreg908
    
When %vreg908 is coalesced into SP but %vreg907 is not, it's likely to emit
an illegal instruction:
  sub sp, r12, #80
    
In Thumb2, we can only use SP as Rd if and only if Rn is also SP. This patch,
when trying remove "%SP<def> = COPY %vreg908", checks if %vreg907 is SP
already. If not, be conservative and do not coalesce %vreg908 into SP.

[zzheng]

Bugzilla: https://llvm.org/bugs/show_bug.cgi?id=23772

[qcolombet]

Hi,

The proposed target hook is not flexible enough IMHO.
What if the reserved register is a source register?
What if the destination MI has several destination register, which one are we looking at?

I believe the right answer would be: provide the ReservedReg and the operand index.

Thanks,
-Quentin

PS: Please upload the context with your patch. This helps reviewing.

[MatzeB]

Hmm I think the RegisterCoalescer is correct. Maybe it would be better
to use a register class for the Rn input that doesn't have sp in it and create new opcodes for the "sub sp" cases.

[qcolombet]

Testcase?

[qcolombet]

Hi Matthias,

I tend to agree with you that the coalescer is correct since the encoding permit it. On the other hand, this is the only pass I am aware of that messed up with the reserved register. E.g., the machine copy propagation pass would not create such pattern.

Cheers,
-Quentin

[MatzeB]

I just looked at the ARM instruction definitions and it seems that the T2I_bin_ii12rs multiclass used for SUB has a no-SP restriction (it is using the rGPR class instead of GPRnopc) but on the Rm input instead of the Rn input, maybe this is simply a specification typo?

[t.p.northover]

Hi,

This looks more like a bug with how we model t2SUBri's register usage to me. For example we still allow someone to assemble "sub r12, sp, #80" with this change. Also, the kind of logic you're trying to add here seems to be completely incidental to whether the register is reserved or not: either the register classes model the constraints of the instruction, or they don't.

I'd probably split up the definitions to create something like Thumb's "tSUBspi" and make the normal variants take and define an rGPR.

Also, there should be a test for this.

Cheers.

Tim.

[qcolombet]

SGTM.

Please fix the encoding of the SUB instruction!

Q.

include/llvm/Target/TargetRegisterInfo.h

                               const TargetRegisterClass *NewRC) const
   { return true; }
 
+  /// \brief No risk to coalescing into reserved physical register if this
+  /// returns true.
+  virtual bool shouldCoalesceReservedPhysReg(unsigned DstReg,
+                                             MachineInstr *DestMI) const
+  { return true; }
+
   //===--------------------------------------------------------------------===//
   /// Debug information queries.
 

lib/CodeGen/RegisterCoalescer.cpp

       }
     }
 
+    if (!TRI->shouldCoalesceReservedPhysReg(DstReg, DestMI)) {
+      DEBUG(dbgs() << "\tSubtarget bailed on coalescing"
+                   << "into reserved physical register.\n");
+      return false;
+    }
+
     // We're going to remove the copy which defines a physical reserved
     // register, so remove its valno, etc.
     DEBUG(dbgs() << "\t\tRemoving phys reg def of " << DstReg << " at "

lib/Target/ARM/ARMBaseRegisterInfo.h

                            int SPAdj, unsigned FIOperandNum,
                            RegScavenger *RS = nullptr) const override;
 
-  /// \brief SrcRC and DstRC will be morphed into NewRC if this returns true
+  /// \brief SrcRC and DstRC will be morphed into NewRC if this returns true.
   bool shouldCoalesce(MachineInstr *MI,
                       const TargetRegisterClass *SrcRC,
                       unsigned SubReg,
                       const TargetRegisterClass *DstRC,
                       unsigned DstSubReg,
                       const TargetRegisterClass *NewRC) const override;
+
+  /// \brief No risk to coalescing into reserved physical register if this
+  /// returns true.
+  bool shouldCoalesceReservedPhysReg(unsigned DestReg,
+                                     MachineInstr *DestMI) const override;
 };
 
 } // end namespace llvm

lib/Target/ARM/ARMBaseRegisterInfo.cpp

   }
   return false;
 }
+
+bool
+ARMBaseRegisterInfo::shouldCoalesceReservedPhysReg(unsigned DstReg,
+                                                   MachineInstr *DestMI) const {
+  // For t2SUBri, t2ADDri, t2SUBri12, and t2ADDri12, Rd is allowed to be SP
+  // if and only if Rn is SP
+  if (DstReg == ARM::SP) {
+    // We're trying to coalesce into SP, if DestMI looks like stack adjustment,
+    // i.e. t2SUBri or t2ADDri, and its Rn is not SP already, be conservative.
+    unsigned Opc = DestMI->getOpcode();
+    switch (Opc) {
+    case ARM::t2SUBri:
+    case ARM::t2ADDri:
+    case ARM::t2SUBri12:
+    case ARM::t2ADDri12: {
+      unsigned SrcReg = DestMI->getOperand(1).getReg();
+      if (!isPhysicalRegister(SrcReg) || (SrcReg != ARM::SP))
+        return false;
+    }
+    default: return true;
+    }
+  }
+  return true;
+}