[TargetRegisterInfo, AArch64] Add target hook for isConstantPhysReg().

[gberry]

Summary

The current implementation of isConstantPhysReg() checks for defs of
physical registers to determine if they are constant. Some
architectures (e.g. AArch64 XZR/WZR) have registers that are constant
and may be used as destinations to indicate the generated value is
discarded, preventing isConstantPhysReg() from returning true. This
change adds a TargetRegisterInfo hook that overrides the no defs check
for cases such as this.

[mcrosier]

Testcase?

[MatzeB]

include/llvm/Target/TargetRegisterInfo.h

498–499 ↗

(On Diff #71373)

This needs a comment (even if it just refers to MachineRegisterInfo::isConstantPhysReg())!
You may want to use MCPhysReg instead of unsigned to indicate it is only valid for physregs.

[gberry]

In D24570#542639, @mcrosier wrote:

Testcase?

I'm looking into that now.

[gberry]

I meant to add this before: I'm interested to hear opinions on whether adding a virtual function call for this is okay, or if this bit should instead say be cached in TargetRegisterInfoDesc like inAllocatableClass is.

Also, this change potentially fixes https://llvm.org/bugs/show_bug.cgi?id=25457

[MatzeB]

In D24570#542806, @gberry wrote:

I meant to add this before: I'm interested to hear opinions on whether adding a virtual function call for this is okay, or if this bit should instead say be cached in TargetRegisterInfoDesc like inAllocatableClass is.

Hard to say in general. Depends on how it is called, whether most implementations are just a few equality comparisons, ...

It looks good enough for todays uses. You could do some measurements though I wouldn't expect any compiletime differences. We can always add more caching later when necessary.

[gberry]

In D24570#542822, @MatzeB wrote:

In D24570#542806, @gberry wrote:

I meant to add this before: I'm interested to hear opinions on whether adding a virtual function call for this is okay, or if this bit should instead say be cached in TargetRegisterInfoDesc like inAllocatableClass is.

Hard to say in general. Depends on how it is called, whether most implementations are just a few equality comparisons, ...

It looks good enough for todays uses. You could do some measurements though I wouldn't expect any compiletime differences. We can always add more caching later when necessary.

That was pretty much my thinking as well.

[gberry]

Added comment and test case

[gberry]

Ping?

[MatzeB]

LGTM, but please simplify the test as much as possible before committing:

test/CodeGen/MIR/AArch64/machine-sink-zr.mir

4–20 ↗

(On Diff #71533)

I believe there is no need for any backreference here and you can simplify this to define void @sinkwzr() { ret void } (see below for how to block names).

24–29 ↗

(On Diff #71533)

I would expect that the default values are fine for those options so you don't need to specify them here.

40 ↗

(On Diff #71533)

I would add a CHECK-LABEL: name: sinkwzr to get the checker on the right function (in case people add more functions to this test in the future).

42 ↗

(On Diff #71533)

You can simply use bb.0: here to avoid the backreference to the IR code (similar with all other basic block names).

llvm/trunk/include/llvm/Target/TargetRegisterInfo.h

public:
   virtual ArrayRef<const char *> getRegMaskNames() const = 0;
 
   /// Returns a bitset indexed by physical register number indicating if a
   /// register is a special register that has particular uses and should be
   /// considered unavailable at all times, e.g. SP, RA. This is
   /// used by register scavenger to determine what registers are free.
   virtual BitVector getReservedRegs(const MachineFunction &MF) const = 0;
 
+  /// Returns true if PhysReg is unallocatable and constant throughout the
+  /// function.  Used by MachineRegisterInfo::isConstantPhysReg().
+  virtual bool isConstantPhysReg(unsigned PhysReg) const { return false; }
+
   /// Prior to adding the live-out mask to a stackmap or patchpoint
   /// instruction, provide the target the opportunity to adjust it (mainly to
   /// remove pseudo-registers that should be ignored).
   virtual void adjustStackMapLiveOutMask(uint32_t *Mask) const { }
 
   /// Return a super-register of the specified register
   /// Reg so its sub-register of index SubIdx is Reg.
   unsigned getMatchingSuperReg(unsigned Reg, unsigned SubIdx,

llvm/trunk/lib/CodeGen/MachineRegisterInfo.cpp

void MachineRegisterInfo::freezeReservedRegs(const MachineFunction &MF) {
   assert(ReservedRegs.size() == getTargetRegisterInfo()->getNumRegs() &&
          "Invalid ReservedRegs vector from target");
 }
 
 bool MachineRegisterInfo::isConstantPhysReg(unsigned PhysReg,
                                             const MachineFunction &MF) const {
   assert(TargetRegisterInfo::isPhysicalRegister(PhysReg));
 
+  const TargetRegisterInfo *TRI = getTargetRegisterInfo();
+  if (TRI->isConstantPhysReg(PhysReg))
+    return true;
+
   // Check if any overlapping register is modified, or allocatable so it may be
   // used later.
-  for (MCRegAliasIterator AI(PhysReg, getTargetRegisterInfo(), true);
+  for (MCRegAliasIterator AI(PhysReg, TRI, true);
        AI.isValid(); ++AI)
     if (!def_empty(*AI) || isAllocatable(*AI))
       return false;
   return true;
 }
 
 /// markUsesInDebugValueAsUndef - Mark every DBG_VALUE referencing the
 /// specified register as undefined which causes the DBG_VALUE to be

llvm/trunk/lib/Target/AArch64/AArch64RegisterInfo.h

public:
   /// i64 argument and an i64 return value)
   ///
   /// Should return NULL in the case that the calling convention does not have
   /// this property
   const uint32_t *getThisReturnPreservedMask(const MachineFunction &MF,
                                              CallingConv::ID) const;
 
   BitVector getReservedRegs(const MachineFunction &MF) const override;
+  bool isConstantPhysReg(unsigned PhysReg) const override;
   const TargetRegisterClass *
   getPointerRegClass(const MachineFunction &MF,
                      unsigned Kind = 0) const override;
   const TargetRegisterClass *
   getCrossCopyRegClass(const TargetRegisterClass *RC) const override;
 
   bool requiresRegisterScavenging(const MachineFunction &MF) const override;
   bool useFPForScavengingIndex(const MachineFunction &MF) const override;

llvm/trunk/lib/Target/AArch64/AArch64RegisterInfo.cpp

bool AArch64RegisterInfo::isReservedReg(const MachineFunction &MF,
   case AArch64::W19:
   case AArch64::X19:
     return hasBasePointer(MF);
   }
 
   return false;
 }
 
+bool AArch64RegisterInfo::isConstantPhysReg(unsigned PhysReg) const {
+  return PhysReg == AArch64::WZR || PhysReg == AArch64::XZR;
+}
+
 const TargetRegisterClass *
 AArch64RegisterInfo::getPointerRegClass(const MachineFunction &MF,
                                       unsigned Kind) const {
   return &AArch64::GPR64RegClass;
 }
 
 const TargetRegisterClass *
 AArch64RegisterInfo::getCrossCopyRegClass(const TargetRegisterClass *RC) const {

llvm/trunk/test/CodeGen/MIR/AArch64/machine-sink-zr.mir

+# RUN: llc -mtriple=aarch64-none-linux-gnu -run-pass machine-sink -o - %s | FileCheck %s
+--- |
+  define void @sinkwzr() { ret void }
+...
+---
+name:            sinkwzr
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gpr32 }
+  - { id: 1, class: gpr32 }
+  - { id: 2, class: gpr32sp }
+  - { id: 3, class: gpr32 }
+  - { id: 4, class: gpr32 }
+body:             |
+  ; Check that WZR copy is sunk into the loop preheader.
+  ; CHECK-LABEL: name: sinkwzr
+  ; CHECK-LABEL: bb.0:
+  ; CHECK-NOT: COPY %wzr
+  bb.0:
+    successors: %bb.3, %bb.1
+    liveins: %w0
+
+    %0 = COPY %w0
+    %1 = COPY %wzr
+    CBZW %0, %bb.3
+
+  ; CHECK-LABEL: bb.1:
+  ; CHECK: COPY %wzr
+
+  bb.1:
+    successors: %bb.2
+
+    B %bb.2
+
+  bb.2:
+    successors: %bb.3, %bb.2
+
+    %2 = PHI %0, %bb.1, %4, %bb.2
+    %w0 = COPY %1
+    %3 = SUBSWri %2, 1, 0, implicit-def dead %nzcv
+    %4 = COPY %3
+    CBZW %3, %bb.3
+    B %bb.2
+
+  bb.3:
+    RET_ReallyLR
+
+...