[MachineCSE] Add new callback for is caller preserved or constant physregs.

[jtony]

Summary

The instructions addis,addi, bl are used to calculate the address of TLS thread local variables. These TLS access code sequences are generated repeatedly every time the thread local variable is accessed. By communicating to Machine CSE that X2 is guaranteed to have the same value within the same function call (so called Caller Preserved Physical Register), the redundant TLS access code sequences are cleaned up.

Note: for the test case included in the patch, before the fix the same TLS access code sequences (addis/addi/bl) is generated twice. After the fix, it is only generated once. The test case tests the new behaviour after the fix.

[kbarton]

lib/CodeGen/MachineCSE.cpp

255 ↗

(On Diff #119806)

Is it not possible to put this check into isConstantPhysReg instead?

[hfinkel]

lib/CodeGen/MachineCSE.cpp

255 ↗

(On Diff #119806)

No, I don't think so. We don't track register dependencies at all (e.g., during instruction scheduling) for constant phys regs. We can't add r2 to that constant set (because we need scheduling to understand the constraints on the r2 save/restore code).

isCallerPreservedPhysReg can only be used if we know that we're not dealing with transformations that can disturb the save/restore sequences. Is this generally safe here?

[jtony]

lib/CodeGen/MachineCSE.cpp

255 ↗

(On Diff #119806)

Thanks Hal for your quick reply. I agree it is not safe to put the isCallerPreservedPhysReg check into isConstantPhysReg. About your question, are you talking about any spill/reload code at all or you are just talking about prologue/epilogue? If you are talking about the prologue/epilogue, that (PEI) happens after the MCSE, so MCSE cannot disturb the save/restore sequences there (in PEI) it should be safe to do so.

[kbarton]

lib/CodeGen/MachineCSE.cpp

255 ↗

(On Diff #119806)

Yes, that's a good point Hal.

Would it be better to then change this into a separate check that could encapsulate both of these concepts (and maybe others) into a single check? In this case, what we really want is to ensure the contents of Reg are not going to change throughout the body of the function, which is slightly different than what isConstantPhysReg provides. If we put this into a separate check, which then contains the checks for isConstantPhysReg and isCallerPreservedPhysReg it might be more clear.

My thinking is that I suspect there are other places where this same check would be useful. By wrapping this into a separate callback it makes it easier to maintain. The downside being the introduction of yet another callback in the backends.

[hfinkel]

LGTM

lib/CodeGen/MachineCSE.cpp

255 ↗

(On Diff #119806)

Thanks Hal for your quick reply. I agree it is not safe to put the isCallerPreservedPhysReg check into isConstantPhysReg. About your question, are you talking about any spill/reload code at all or you are just talking about prologue/epilogue?

I'm talking about any spill/reload code at all. I think this is okay, because...

isCallerPreservedPhysReg is defined as follows:

/// Physical registers that may be modified within a function but are
/// guaranteed to be restored before any uses. This is useful for targets that
/// have call sequences where a GOT register may be updated by the caller
/// prior to a call and is guaranteed to be restored (also by the caller)
/// after the call. 
virtual bool isCallerPreservedPhysReg(unsigned PhysReg,
                                      const MachineFunction &MF) const {
  return false;
}

So what we know about such a register is that it is the same at any use. However, there may be defs of the register that are required in order to maintain that invariant. The defs are going to be loads, for example, and if you ignore the fact that these define a physical register, then what prevents them from being CSE'd? So we can't do that. However, the change here is only for uses, not defs, so I think we're okay. I think, however, you specifically can't make the corresponding change in the loop below for defs.

255 ↗

(On Diff #119806)

If we put this into a separate check, which then contains the checks for isConstantPhysReg and isCallerPreservedPhysReg it might be more clear.

We could make a utility member function contains both calls. Something like:

bool isCallerPreservedOrConstPhysReg(unsigned PhysReg, const MachineFunction &MF) const {
  const MachineRegisterInfo &MRI = MF.getRegInfo();
  return MRI.isConstantPhysReg(PhysReg) || isCallerPreservedPhysReg(PhysReg, MF);

}

This seems reasonable.

[hfinkel]

Oh, and please retitle this before you commit. You should talk about caller-preserved registers, not TLS.

llvm/trunk/include/llvm/CodeGen/MachineRegisterInfo.h

public:
   void clearKillFlags(unsigned Reg) const;
 
   void dumpUses(unsigned RegNo) const;
 
   /// Returns true if PhysReg is unallocatable and constant throughout the
   /// function. Writing to a constant register has no effect.
   bool isConstantPhysReg(unsigned PhysReg) const;
 
+  /// Returns true if either isConstantPhysReg or TRI->isCallerPreservedPhysReg
+  /// returns true. This is a utility member function.
+  bool isCallerPreservedOrConstPhysReg(unsigned PhysReg) const;
+
   /// Get an iterator over the pressure sets affected by the given physical or
   /// virtual register. If RegUnit is physical, it must be a register unit (from
   /// MCRegUnitIterator).
   PSetIterator getPressureSets(unsigned RegUnit) const;
 
   //===--------------------------------------------------------------------===//
   // Virtual Register Info
   //===--------------------------------------------------------------------===//

llvm/trunk/lib/CodeGen/MachineCSE.cpp

bool MachineCSE::hasLivePhysRegDefUses(const MachineInstr *MI,
   for (const MachineOperand &MO : MI->operands()) {
     if (!MO.isReg() || MO.isDef())
       continue;
     unsigned Reg = MO.getReg();
     if (!Reg)
       continue;
     if (TargetRegisterInfo::isVirtualRegister(Reg))
       continue;
-    // Reading constant physregs is ok.
-    if (!MRI->isConstantPhysReg(Reg))
+    // Reading either caller preserved or constant physregs is ok.
+    if (!MRI->isCallerPreservedOrConstPhysReg(Reg))
       for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI)
         PhysRefs.insert(*AI);
   }
 
   // Next, collect all defs into PhysDefs.  If any is already in PhysRefs
   // (which currently contains only uses), set the PhysUseDef flag.
   PhysUseDef = false;
   MachineBasicBlock::const_iterator I = MI; I = std::next(I);

llvm/trunk/lib/CodeGen/MachineRegisterInfo.cpp

bool MachineRegisterInfo::isConstantPhysReg(unsigned PhysReg) const {
   // used later.
   for (MCRegAliasIterator AI(PhysReg, TRI, true);
        AI.isValid(); ++AI)
     if (!def_empty(*AI) || isAllocatable(*AI))
       return false;
   return true;
 }
 
+bool
+MachineRegisterInfo::isCallerPreservedOrConstPhysReg(unsigned PhysReg) const {
+  const TargetRegisterInfo *TRI = getTargetRegisterInfo();
+  return isConstantPhysReg(PhysReg) ||
+      TRI->isCallerPreservedPhysReg(PhysReg, *MF);
+}
+
 /// markUsesInDebugValueAsUndef - Mark every DBG_VALUE referencing the
 /// specified register as undefined which causes the DBG_VALUE to be
 /// deleted during LiveDebugVariables analysis.
 void MachineRegisterInfo::markUsesInDebugValueAsUndef(unsigned Reg) const {
   // Mark any DBG_VALUE that uses Reg as undef (but don't delete it.)
   MachineRegisterInfo::use_instr_iterator nextI;
   for (use_instr_iterator I = use_instr_begin(Reg), E = use_instr_end();
        I != E; I = nextI) {

llvm/trunk/test/CodeGen/PowerPC/MCSE-caller-preserved-reg.ll

+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
+; The instructions addis,addi, bl are used to calculate the address of TLS
+; thread local variables. These TLS access code sequences are generated
+; repeatedly every time the thread local variable is accessed. By communicating
+; to Machine CSE that X2 is guaranteed to have the same value within the same
+; function call (so called Caller Preserved Physical Register), the redudant
+; TLS access code sequences are cleaned up.
+
+%"struct.CC::TT" = type { i64, i32 }
+%class.CC = type { %struct.SS }
+%struct.SS = type { void ()* }
+
+@_ZN2CC2ccE = external thread_local global %"struct.CC::TT", align 8
+
+define noalias i8* @_ZN2CC3funEv(%class.CC* %this) {
+; CHECK-LABEL: _ZN2CC3funEv:
+; CHECK:    mflr 0
+; CHECK-NEXT:    std 0, 16(1)
+; CHECK-NEXT:    stdu 1, -48(1)
+; CHECK-NEXT:    .cfi_def_cfa_offset 48
+; CHECK-NEXT:    .cfi_offset lr, 16
+; CHECK-NEXT:    .cfi_offset r30, -16
+; CHECK-NEXT:    std 30, 32(1)
+; CHECK-NEXT:    mr 30, 3
+; CHECK-NEXT:    ld 12, 0(30)
+; CHECK-NEXT:    std 2, 24(1)
+; CHECK-NEXT:    mtctr 12
+; CHECK-NEXT:    bctrl
+; CHECK-NEXT:    ld 2, 24(1)
+; CHECK-NEXT:    addis 3, 2, _ZN2CC2ccE@got@tlsgd@ha
+; CHECK-NEXT:    addi 3, 3, _ZN2CC2ccE@got@tlsgd@l
+; CHECK-NEXT:    bl __tls_get_addr(_ZN2CC2ccE@tlsgd)
+; CHECK-NEXT:    nop
+; CHECK-NEXT:    ld 4, 0(3)
+; CHECK-NEXT:    cmpldi 4, 0
+; CHECK-NEXT:    beq 0, .LBB0_2
+; CHECK:    addi 4, 3, 8
+; CHECK-NEXT:    mr 3, 30
+; CHECK-NEXT:    bl _ZN2CC3barEPi
+; CHECK-NEXT:    nop
+; CHECK:    ld 30, 32(1)
+; CHECK-NEXT:    li 3, 0
+; CHECK-NEXT:    addi 1, 1, 48
+; CHECK-NEXT:    ld 0, 16(1)
+; CHECK-NEXT:    mtlr 0
+; CHECK-NEXT:    blr
+entry:
+  %foo = getelementptr inbounds %class.CC, %class.CC* %this, i64 0, i32 0, i32 0
+  %0 = load void ()*, void ()** %foo, align 8
+  tail call void %0()
+  %1 = load i64, i64* getelementptr inbounds (%"struct.CC::TT", %"struct.CC::TT"* @_ZN2CC2ccE, i64 0, i32 0)
+  %tobool = icmp eq i64 %1, 0
+  br i1 %tobool, label %if.end, label %if.then
+
+if.then:
+  tail call void @_ZN2CC3barEPi(%class.CC* nonnull %this, i32* getelementptr inbounds (%"struct.CC::TT", %"struct.CC::TT"* @_ZN2CC2ccE, i64 0, i32 1))
+  br label %if.end
+
+if.end:
+  ret i8* null
+}
+
+declare void @_ZN2CC3barEPi(%class.CC*, i32*)

llvm/trunk/test/CodeGen/PowerPC/licm-tocReg.ll

 ; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
 
-; The instructions ADDIStocHA/LDtocL are used to calculate the address of 
-; globals. The ones that are in bb.3.if.end could not be hoisted by Machine 
+; The instructions ADDIStocHA/LDtocL are used to calculate the address of
+; globals. The ones that are in bb.3.if.end could not be hoisted by Machine
 ; LICM due to BCTRL_LDinto_toc in bb2.if.then.  This call causes the compiler
 ; to insert a save TOC to stack before the call and load into X2 to restore TOC
-; after. By communicating to Machine LICM that X2 is guaranteed to have the 
+; after. By communicating to Machine LICM that X2 is guaranteed to have the
 ; same value before and after BCTRL_LDinto_toc, these instructions can be
 ; hoisted out of bb.3.if.end to outside of the loop.
 
 ; Pre Machine LICM MIR
 ;
-;body:             
+;body:
 ;  bb.0.entry:
 ;    successors: %bb.2.if.then(0x40000000), %bb.3.if.end(0x40000000)
 ;    liveins: %x3
-;  
+;
 ;    %4 = COPY %x3
 ;    %5 = ADDIStocHA %x2, @ga
 ;    %6 = LDtocL @ga, killed %5 :: (load 8 from got)
 ;    %7 = LWZ 0, %6 :: (volatile dereferenceable load 4 from @ga)
 ;    %8 = ADDIStocHA %x2, @gb
 ;    %9 = LDtocL @gb, killed %8 :: (load 8 from got)
 ;    %10 = LWZ 0, killed %9 :: (volatile dereferenceable load 4 from @gb)
 ;    %0 = LWZ 0, %6 :: (volatile dereferenceable load 4 from @ga)
 ;    %11 = CMPW killed %7, killed %10
 ;    BCC 44, killed %11, %bb.2.if.then
 ;    B %bb.3.if.end
-;  
+;
 ;  bb.2.if.then:
 ;    %1 = PHI %0, %bb.0.entry, %3, %bb.3.if.end
 ;    ADJCALLSTACKDOWN 32, 0, implicit-def dead %r1, implicit %r1
 ;    %20 = COPY %x2
 ;    STD %20, 24, %x1 :: (store 8 into stack + 24)
 ;    %21 = EXTSW_32_64 %1
 ;    %x3 = COPY %21
 ;    %x12 = COPY %4
 ;    MTCTR8 %4, implicit-def %ctr8
 ;    BCTRL8_LDinto_toc 24, %x1, csr_svr464_altivec, implicit-def dead %lr8, implicit-def dead %x2, implicit %ctr8, implicit %rm, implicit %x3, implicit %x12, implicit %x2, implicit-def %r1, implicit-def %x3
 ;    ADJCALLSTACKUP 32, 0, implicit-def dead %r1, implicit %r1
 ;    %22 = COPY %x3
 ;    %x3 = COPY %22
 ;    BLR8 implicit %lr8, implicit %rm, implicit %x3
-;  
+;
 ;  bb.3.if.end:
 ;    successors: %bb.2.if.then(0x04000000), %bb.3.if.end(0x7c000000)
-;  
+;
 ;    %2 = PHI %0, %bb.0.entry, %3, %bb.3.if.end
 ;    %12 = ADDI %2, 1
 ;    %13 = ADDIStocHA %x2, @ga
 ;    %14 = LDtocL @ga, killed %13 :: (load 8 from got)
 ;    STW killed %12, 0, %14 :: (volatile store 4 into @ga)
 ;    %15 = LWZ 0, %14 :: (volatile dereferenceable load 4 from @ga)
 ;    %16 = ADDIStocHA %x2, @gb
 ;    %17 = LDtocL @gb, killed %16 :: (load 8 from got)
 ;    %18 = LWZ 0, killed %17 :: (volatile dereferenceable load 4 from @gb)
 ;    %3 = LWZ 0, %14 :: (volatile dereferenceable load 4 from @ga)
 ;    %19 = CMPW killed %15, killed %18
 ;    BCC 44, killed %19, %bb.2.if.then
 ;    B %bb.3.if.end
 
 @ga = external global i32, align 4
 @gb = external global i32, align 4
 
-; Function Attrs: nounwind
 define signext i32 @test(i32 (i32)* nocapture %FP) local_unnamed_addr #0 {
 ; CHECK-LABEL: test:
 ; CHECK:       # BB#0: # %entry
-; CHECK-NEXT:    addis 4, 2, .LC0@toc@ha
-; CHECK-NEXT:    addis 5, 2, .LC1@toc@ha
-; CHECK-NEXT:    ld 4, .LC0@toc@l(4)
-; CHECK-NEXT:    ld 5, .LC1@toc@l(5)
-; CHECK-NEXT:    lwz 6, 0(4)
-; CHECK-NEXT:    lwz 5, 0(5)
-; CHECK-NEXT:    cmpw 6, 5
-; CHECK-NEXT:    lwz 5, 0(4)
+; CHECK-NEXT:    addis 6, 2, .LC0@toc@ha
+; CHECK-NEXT:    addis 4, 2, .LC1@toc@ha
+; CHECK-NEXT:    ld 5, .LC1@toc@l(4)
+; CHECK-NEXT:    ld 6, .LC0@toc@l(6)
+; CHECK-NEXT:    lwz 4, 0(5)
+; CHECK-NEXT:    lwz 7, 0(6)
+; CHECK-NEXT:    cmpw 4, 7
+; CHECK-NEXT:    lwz 7, 0(5)
 ; CHECK-NEXT:    mr 4, 3
-; CHECK-NEXT:    bgt 0, .LBB0_3
-; CHECK-NEXT:  # BB#1:
-; CHECK-NEXT:    addis 3, 2, .LC0@toc@ha
-; CHECK-NEXT:    addis 6, 2, .LC1@toc@ha
-; CHECK-NEXT:    ld 3, .LC0@toc@l(3)
-; CHECK-NEXT:    ld 6, .LC1@toc@l(6)
+; CHECK-NEXT:    bgt 0, .LBB0_2
+; CHECK-NOT:    addis {{[0-9]+}}, 2, .LC0@toc@ha
+; CHECK-NOT:    addis {{[0-9]+}}, 2, .LC1@toc@ha
 ; CHECK-NEXT:    .p2align 5
-; CHECK-NEXT:  .LBB0_2: # %if.end
+; CHECK-NEXT:  .LBB0_1: # %if.end
 ; CHECK-NOT:    addis {{[0-9]+}}, 2, .LC0@toc@ha
 ; CHECK-NOT:    addis {{[0-9]+}}, 2, .LC1@toc@ha
 ; CHECK:    blr
 entry:
   %0 = load volatile i32, i32* @ga, align 4
   %1 = load volatile i32, i32* @gb, align 4
   %cmp1 = icmp sgt i32 %0, %1
   %2 = load volatile i32, i32* @ga, align 4