LiveIntervalAnalysis: Fix alias regunit reserved definition

[MatzeB]

Summary

A register in CodeGen can be marked as reserved: In that case we
consider the register always live and do not use (or rather ignore)
kill/dead/undef operand flags.

LiveIntervalAnalysis however tracks liveness per register unit (not per
register). We already needed adjustments for this in r292871 to deal
with super/sub registers. However I did not look at aliased register
there. Looking at ARM:

FPSCR (regunits FPSCR, FPSCR~FPSCR_NZCV) aliases with FPSCR_NZCV
(regunits FPSCR_NZCV, FPSCR~FPSCR_NZCV) hence they share a register unit
(FPSCR~FPSCR_NZCV) that represents the aliased parts of the registers.
This shared register unit was previously considered non-reserved,
however given that we uses of the reserved FPSCR potentially violate
some rules (like uses without defs) we should make FPSCR~FPSCR_NZCV
reserved too and stop tracking liveness for it.

This patch:

• Defines a register unit as reserved when: At least for one root register, the root register and all its super registers are reserved.
• Adjust LiveIntervals::computeRegUnitRange() for new reserved definition.
• Add MachineRegisterInfo::isReservedRegUnit() to have a canonical way of testing.
• Stop computing LiveRanges for reserved register units in HMEditor even with UpdateFlags enabled.
• Skip verification of uses of reserved reg units in the machine verifier (this usually didn't happen because there would be no cached liverange but there is no guarantee for that and I would run into this case before the HMEditor tweak, so may as well fix the verifier too).

Note that this should only affect ARMs FPSCR/FPSCR_NZCV registers today;
aliased registers are rarely used, the only other cases are hexagons
P0-P3/P3_0 and C8/USR pairs which are not mixing reserved/non-reserved
registers in an alias.

[kparzysz]

lib/CodeGen/LiveIntervalAnalysis.cpp

279

Should this still happen if Reg is reserved? For reserved units, the LR will only have dead defs. Is that the intent?

[MatzeB]

lib/CodeGen/LiveIntervalAnalysis.cpp

279

The code in RegisterCoalescer::joinReservedPhysReg() expects it that way. The comment there reads:

// The live range of the reserved register will look like a set of dead defs
// - we don't properly track the live range of reserved registers.

[kparzysz]

On a somewhat unrelated note---is it valid on AArch64 to reserve W1_W2 without reserving W1 and W2? If so, it would lead to a situation where a reserved register does not contain any reserved units. If not, it would require one or both of W0_W1 and W2_W3 to also be reserved.

[MatzeB]

Thanks for the review!

In D37356#858909, @kparzysz wrote:

On a somewhat unrelated note---is it valid on AArch64 to reserve W1_W2 without reserving W1 and W2? If so, it would lead to a situation where a reserved register does not contain any reserved units. If not, it would require one or both of W0_W1 and W2_W3 to also be reserved.

Yes this is allowed. That is exactly the situation I worked on in r292871. A typical situation you see in GPU targets is one or two registers reserved as stackpointer or similar ABI purposes, requiring that we also reserve all the tuples involving that register (usually done with markSuperRegs() in the targets getReservedRegs()). However just because one register in the tuple is reserved doesn't necessarily mean the other registers (when seen independently of the tuple) are reserved. Hence the logic in here that only considers a register unit as reserved if the register unit root register and all the super registers are reserved. This is for the situation of super/sub registers!

It just turned out that for aliased registers (the only situation in which we have more than 1 register unit root) requiring all register unit roots to be reserved, in order for the register unit to be reserved wasn't too helpful in ARMs case, where the reserved register was sometimes used in ways that would be invalid for a non-reserved register. So we wouldn't be able to compute liveness for the special register unit that representing the alias.

include/llvm/CodeGen/MachineRegisterInfo.h

#endif
   /// isReserved - Returns true when PhysReg is a reserved register.
   ///
   /// Reserved registers may belong to an allocatable register class, but the
   /// target has explicitly requested that they are not used.
   bool isReserved(unsigned PhysReg) const {
     return getReservedRegs().test(PhysReg);
   }
 
+  /// Returns true when the given register unit is considered reserved.
+  ///
+  /// Register units are considered reserved when for at least one of their
+  /// root registers, the root register and all super registers are reserved.
+  /// This currently iterates the register hierarchy and may be slower than
+  /// expected.
+  bool isReservedRegUnit(unsigned Unit) const;
+
   /// isAllocatable - Returns true when PhysReg belongs to an allocatable
   /// register class and it hasn't been reserved.
   ///
   /// Allocatable registers may show up in the allocation order of some virtual
   /// register, so a register allocator needs to track its liveness and
   /// availability.
   bool isAllocatable(unsigned PhysReg) const {
     return getTargetRegisterInfo()->isInAllocatableClass(PhysReg) &&

lib/CodeGen/LiveIntervalAnalysis.cpp

void LiveIntervals::computeRegUnitRange(LiveRange &LR, unsigned Unit) {
   assert(LRCalc && "LRCalc not initialized.");
   LRCalc->reset(MF, getSlotIndexes(), DomTree, &getVNInfoAllocator());
 
   // The physregs aliasing Unit are the roots and their super-registers.
   // Create all values as dead defs before extending to uses. Note that roots
   // may share super-registers. That's OK because createDeadDefs() is
   // idempotent. It is very rare for a register unit to have multiple roots, so
   // uniquing super-registers is probably not worthwhile.
-  bool IsReserved = true;
+  bool IsReserved = false;
   for (MCRegUnitRootIterator Root(Unit, TRI); Root.isValid(); ++Root) {
+    bool IsRootReserved = true;
     for (MCSuperRegIterator Super(*Root, TRI, /*IncludeSelf=*/true);
          Super.isValid(); ++Super) {
       unsigned Reg = *Super;
       if (!MRI->reg_empty(Reg))
         LRCalc->createDeadDefs(LR, Reg);
       // A register unit is considered reserved if all its roots and all their
       // super registers are reserved.
       if (!MRI->isReserved(Reg))
-        IsReserved = false;
+        IsRootReserved = false;
     }
+    IsReserved |= IsRootReserved;
   }
+  assert(IsReserved == MRI->isReservedRegUnit(Unit) &&
+         "reserved computation mismatch");
 
   // Now extend LR to reach all uses.
   // Ignore uses of reserved registers. We only track defs of those.
   if (!IsReserved) {
     for (MCRegUnitRootIterator Root(Unit, TRI); Root.isValid(); ++Root) {
       for (MCSuperRegIterator Super(*Root, TRI, /*IncludeSelf=*/true);
            Super.isValid(); ++Super) {
         unsigned Reg = *Super;
HMEditor(LiveIntervals& LIS, const MachineRegisterInfo& MRI,
     : LIS(LIS), MRI(MRI), TRI(TRI), OldIdx(OldIdx), NewIdx(NewIdx),
       UpdateFlags(UpdateFlags) {}
 
   // FIXME: UpdateFlags is a workaround that creates live intervals for all
   // physregs, even those that aren't needed for regalloc, in order to update
   // kill flags. This is wasteful. Eventually, LiveVariables will strip all kill
   // flags, and postRA passes will use a live register utility instead.
   LiveRange *getRegUnitLI(unsigned Unit) {
-    if (UpdateFlags)
+    if (UpdateFlags && !MRI.isReservedRegUnit(Unit))
       return &LIS.getRegUnit(Unit);
     return LIS.getCachedRegUnit(Unit);
   }
 
   /// Update all live ranges touched by MI, assuming a move from OldIdx to
   /// NewIdx.
   void updateAllRanges(MachineInstr *MI) {
     DEBUG(dbgs() << "handleMove " << OldIdx << " -> " << NewIdx << ": " << *MI);

lib/CodeGen/MachineRegisterInfo.cpp

void MachineRegisterInfo::setCalleeSavedRegs(ArrayRef<MCPhysReg> CSRs) {
   for (MCPhysReg Reg : CSRs)
     UpdatedCSRs.push_back(Reg);
 
   // Zero value represents the end of the register list
   // (no more registers should be pushed).
   UpdatedCSRs.push_back(0);
   IsUpdatedCSRsInitialized = true;
 }
+
+bool MachineRegisterInfo::isReservedRegUnit(unsigned Unit) const {
+  const TargetRegisterInfo *TRI = getTargetRegisterInfo();
+  for (MCRegUnitRootIterator Root(Unit, TRI); Root.isValid(); ++Root) {
+    bool IsRootReserved = true;
+    for (MCSuperRegIterator Super(*Root, TRI, /*IncludeSelf=*/true);
+         Super.isValid(); ++Super) {
+      unsigned Reg = *Super;
+      if (!isReserved(Reg)) {
+        IsRootReserved = false;
+        break;
+      }
+    }
+    if (IsRootReserved)
+      return true;
+  }
+  return false;
+}

lib/CodeGen/MachineVerifier.cpp

if (MO->readsReg()) {
     }
 
     // Check LiveInts liveness and kill.
     if (LiveInts && !LiveInts->isNotInMIMap(*MI)) {
       SlotIndex UseIdx = LiveInts->getInstructionIndex(*MI);
       // Check the cached regunit intervals.
       if (TargetRegisterInfo::isPhysicalRegister(Reg) && !isReserved(Reg)) {
         for (MCRegUnitIterator Units(Reg, TRI); Units.isValid(); ++Units) {
+          if (MRI->isReservedRegUnit(*Units))
+            continue;
           if (const LiveRange *LR = LiveInts->getCachedRegUnit(*Units))
             checkLivenessAtUse(MO, MONum, UseIdx, *LR, *Units);
         }
       }
 
       if (TargetRegisterInfo::isVirtualRegister(Reg)) {
         if (LiveInts->hasInterval(Reg)) {
           // This is a virtual register interval.

test/CodeGen/ARM/no-fpscr-liveness.ll

+; RUN: llc -o - %s | FileCheck %s
+; Make sure we do not try to compute liveness for FPSCR which in this case
+; is read before being written to (this is fine because becase FPSCR is
+; reserved).
+target triple = "thumbv7s-apple-ios"
+
+%struct.wibble = type { double }
+
+@global = common global i32 0, align 4
+@global.1 = common global i32 0, align 4
+
+; CHECK-LABEL: eggs:
+; CHECK: sub sp, #8
+; VMRS instruction comes before any other instruction writing FPSCR:
+; CHECK-NEXT: vmrs r0, fpscr
+; ...
+; CHECK: add sp, #8
+; CHECK: bx lr
+define i32 @eggs(double* nocapture readnone %arg) {
+bb:
+  %tmp = alloca %struct.wibble, align 4
+  %tmp1 = bitcast %struct.wibble* %tmp to i8*
+  %tmp2 = tail call i32 @llvm.flt.rounds()
+  %tmp3 = ptrtoint %struct.wibble* %tmp to i32
+  %tmp4 = sitofp i32 %tmp3 to double
+  %tmp5 = fmul double %tmp4, 0x0123456789ABCDEF
+  %tmp6 = fptosi double %tmp5 to i32
+  %tmp7 = fcmp une double %tmp5, 0.000000e+00
+  %tmp8 = sitofp i32 %tmp6 to double
+  %tmp9 = fcmp une double %tmp5, %tmp8
+  %tmp10 = and i1 %tmp7, %tmp9
+  %tmp11 = sext i1 %tmp10 to i32
+  %tmp12 = add nsw i32 %tmp11, %tmp6
+  store i32 %tmp12, i32* @global, align 4
+  %tmp13 = icmp ne i32 %tmp12, 0
+  %tmp14 = icmp ne i32 %tmp2, 0
+  %tmp15 = and i1 %tmp14, %tmp13
+  br i1 %tmp15, label %bb16, label %bb18
+
+bb16:                                             ; preds = %bb
+  %tmp17 = load i32, i32* @global.1, align 4
+  br label %bb18
+
+bb18:                                             ; preds = %bb16, %bb
+  ret i32 undef
+}
+
+declare i32 @llvm.flt.rounds()
+declare i32 @zot(...)