[SystemZ] Mark unsaved argument R6 as live throughout function

[jonpa]

Summary

For historical reasons, the R6 register is a callee saved argument register. This means that if it is used to pass an argument to a function that does not clobber it, it is live throughout the function.

This patch makes sure that in this special case any kill flags of it are removed, all MBBs have it in their live-in lists and all return instructions use it.

[efriedma]

This seems like the right approach. It feels weird, but I can't think of anything better.

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

356

Not sure I understand what the LowGPR && SystemZMC::getFirstReg(LowGPR) > SystemZMC::getFirstReg(SystemZ::R6D) is doing. We should be checking whether the register was spilled in the prologue.

llvm/test/CodeGen/SystemZ/frame-26.ll

6 ↗

(On Diff #298324)

Probably want an MIR testcase; smaller, and more likely to continue to hit the case in question in the future.

[uweigand]

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

356

That's exactly what this check does: we use a STORE MULTIPLE instruction to spill the range of registers from LowGPR up to r15. So "LowGPR > r6" is equivalent to "r6 is not spilled in the prologue".

[jonpa]

Updated per review to use a MIR test instead.

I cleaned it up as best I could, but I could not get rid of the IsSSA flag being set, but it seems generally ignored in tests.. For instance AArch64/wineh-frame3.mir also has this problem. It doesn't seem to matter though...

bin/llc -o - ../llvm/test/CodeGen/AArch64/wineh-frame3.mir -mtriple=aarch64-windows -start-before=prologepilog -print-after=prologepilog                                                                                                      
        .text                                                                                                          
        .file   "wineh-frame3.mir"                                                                                     
# *** IR Dump After Prologue/Epilogue Insertion & Frame Finalization ***:                                              
# Machine code for function test: IsSSA, NoPHIs, TracksLiveness, NoVRegs

[efriedma]

LGTM

[uweigand]

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

369

Just one question here: do we need to explicitly add this use? If I followed your description in the bugzilla correctly, all callee-saved register are implicitly considered used at the end of the return block?

[jonpa]

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

369

Good question... It seems both LiveRegUnits and LivePhysRegs classes do special-treat the return blocks so it might be that this is implicitly modelled in the IR. RegScavenger uses LiveRegUnits (I don't know why there are two different classes for this) so in this particular case the use operands are not needed.

I now see a comment at LivePhysRegs::addLiveOutsNoPristines():

// Return blocks are a special case because we currently don't mark up
// return instructions completely: specifically, there is no explicit
// use for callee-saved registers. So we add all callee saved registers
// that are saved and restored (somewhere). This does not include
// callee saved registers that are unused and hence not saved and
// restored; they are called pristine.
// FIXME: PEI should add explicit markings to return instructions
// instead of implicitly handling them here.

It seems there is some intent on moving towards explicit use operands on return instructions instead?

The notion of a "pristine" register meaning an unused callee saved reg, makes me think we may want to be explicit about R6 since that seems to imply it is completely unused, which may not be true...?

[efriedma]

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

369

I guess LivePhysRegs/LiveRegUnits will treat r6 as "pristine" since we don't save/restore it. I guess, thinking about it a bit more, that might means it's okay to skip marking up the liveins and the return instructions; it's implicitly live anyway.

As far as I can tell, nothing cares if something reads a pristine register; it's only a problem if an instruction writes/kills one. But there's so little code that runs this late in the pipeline, I'm not sure how consistent everything really is, anyway.

[jonpa]

• Removed the adding of R6 to live-in lists and return instructions.

• Changed the check to use getRestoreGPRRegs() instead of getSpillGPRRegs(). These may differ for a vararg function, where a vararg R6 is spilled but not restored. In that case it is still unclobbered and live throughout the function. (The spilling of vararg r6 will first be marked with kill, but this patch will remove it).

• Changed the check to compare against SystemZ::R6 directly since that should be the lowest restored GPR.

[uweigand]

See inline comment; otherwise this LGTM.

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

357

Just a minor nit, maybe the loop can be written more compact using C++11 features like:

for (auto &I : MRI->use_nodbg_operands(SystemZ::R6D))

This should be equivalent as far as I can see.

llvm/lib/Target/SystemZ/SystemZFrameLowering.cpp

bool SystemZFrameLowering::restoreCalleeSavedRegisters(
 
   return true;
 }
 
 void SystemZFrameLowering::
 processFunctionBeforeFrameFinalized(MachineFunction &MF,
                                     RegScavenger *RS) const {
   MachineFrameInfo &MFFrame = MF.getFrameInfo();
+  SystemZMachineFunctionInfo *ZFI = MF.getInfo<SystemZMachineFunctionInfo>();
+  MachineRegisterInfo *MRI = &MF.getRegInfo();
   bool BackChain = MF.getFunction().hasFnAttribute("backchain");
 
   if (!usePackedStack(MF) || BackChain)
     // Create the incoming register save area.
     getOrCreateFramePointerSaveIndex(MF);
 
   // Get the size of our stack frame to be allocated ...
   uint64_t StackSize = (MFFrame.estimateStackSize(MF) +
processFunctionBeforeFrameFinalized(MachineFunction &MF,
   if (!isUInt<12>(MaxReach)) {
     // We may need register scavenging slots if some parts of the frame
     // are outside the reach of an unsigned 12-bit displacement.
     // Create 2 for the case where both addresses in an MVC are
     // out of range.
     RS->addScavengingFrameIndex(MFFrame.CreateStackObject(8, Align(8), false));
     RS->addScavengingFrameIndex(MFFrame.CreateStackObject(8, Align(8), false));
   }
+
+  // If R6 is used as an argument register it is still callee saved. If it in
+  // this case is not clobbered (and restored) it should never be marked as
+  // killed.
+  if (MF.front().isLiveIn(SystemZ::R6D) &&
+      ZFI->getRestoreGPRRegs().LowGPR != SystemZ::R6D) {
+    using use_iterator = MachineRegisterInfo::use_nodbg_iterator;
+    use_iterator E = MRI->use_nodbg_end();
+    for (use_iterator I = MRI->use_nodbg_begin(SystemZ::R6D); I != E; ++I)
+      I->setIsKill(false);
+  }
 }
 
 // Emit instructions before MBBI (in MBB) to add NumBytes to Reg.
 static void emitIncrement(MachineBasicBlock &MBB,
                           MachineBasicBlock::iterator &MBBI, const DebugLoc &DL,
                           Register Reg, int64_t NumBytes,
                           const TargetInstrInfo *TII) {
   while (NumBytes) {
     unsigned Opcode;
     int64_t ThisVal = NumBytes;
     if (isInt<16>(NumBytes))
       Opcode = SystemZ::AGHI;
     else {
       Opcode = SystemZ::AGFI;
       // Make sure we maintain 8-byte stack alignment.
       int64_t MinVal = -uint64_t(1) << 31;
       int64_t MaxVal = (int64_t(1) << 31) - 8;
       if (ThisVal < MinVal)

llvm/test/CodeGen/SystemZ/frame-26.mir

+# RUN: llc -mtriple=s390x-linux-gnu -start-before=prologepilog %s -o - -print-after=prologepilog \
+# RUN:  -verify-machineinstrs 2>&1 | FileCheck %s
+# REQUIRES: asserts
+#
+# Test that R6 when used for an argument is modelled as being live throughout
+# the function when not saved in the prologue..
+
+# CHECK:      # *** IR Dump After Prologue/Epilogue Insertion & Frame Finalization ***:
+# CHECK-NEXT: # Machine code for function fun0:
+# CHECK-LABEL: bb.0:
+# CHECK: liveins:{{.*}} $r6d
+# CHECK: STMG killed $r7d, killed $r15d
+# CHECK: STG renamable $r6d
+# CHECK: End machine code for function fun0.
+
+
+--- |
+  
+  @g_181 = external dso_local global i32, align 4
+  @g_1390 = external dso_local constant i64*, align 8
+  
+  define internal i8 @fun0(i8 %arg, i8 %arg1, i32 %arg2, i8 %arg3, i32* %arg4, float %F0, float %F1) #0 {
+    ret i8 0
+  }
+  
+  ; Same function but in a single block which will make the verifier complain
+  ; if R6 is killed by the original store before the point where the
+  ; RegScavenger inserts its (killing) store of R6.
+  define internal i8 @fun1(i8 %arg, i8 %arg1, i32 %arg2, i8 %arg3, i32* %arg4) #0 {
+    ret i8 0
+  }
+  
+  attributes #0 = { "frame-pointer"="all" }
+
+...
+---
+name:            fun0
+alignment:       16
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r6d' }
+  - { reg: '$f0s' }
+  - { reg: '$f2s' }
+frameInfo:
+  maxAlignment:    8
+stack:
+  - { id: 0, size: 96, alignment: 8 }
+  - { id: 1, size: 1960, alignment: 8 }
+  - { id: 2, size: 8, alignment: 8 }
+  - { id: 3, size: 320, alignment: 8 }
+  - { id: 4, size: 4, alignment: 4 }
+  - { id: 5, size: 8, alignment: 8 }
+  - { id: 6, size: 8, alignment: 8 }
+  - { id: 7, size: 4, alignment: 4 }
+  - { id: 8, size: 8, alignment: 8 }
+  - { id: 9, size: 4, alignment: 4 }
+  - { id: 10, size: 8, alignment: 8 }
+  - { id: 11, size: 8, alignment: 8 }
+  - { id: 12, size: 8, alignment: 8 }
+  - { id: 13, size: 8, alignment: 8 }
+  - { id: 14, size: 24, alignment: 4 }
+  - { id: 15, size: 4, alignment: 4 }
+  - { id: 16, size: 1792, alignment: 8 }
+  - { id: 17, size: 8, alignment: 8 }
+  - { id: 18, size: 8, alignment: 8 }
+  - { id: 19, size: 8, alignment: 8 }
+  - { id: 20, size: 1, alignment: 2 }
+  - { id: 21, size: 672, alignment: 8 }
+  - { id: 22, size: 4, alignment: 4 }
+  - { id: 23, size: 4, alignment: 4 }
+  - { id: 24, size: 4, alignment: 4 }
+  - { id: 25, size: 64, alignment: 8 }
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $f0s, $f2s, $r6d
+  
+    STG killed renamable $r6d, undef renamable $r1d, 0, $noreg :: (store 8 into `i32** undef`)
+    renamable $r0d = LARL @g_181
+    nofpexcept CEBR renamable $f0s, renamable $f2s, implicit-def $cc, implicit $fpc
+    STG renamable $r0d, undef renamable $r1d, 0, $noreg :: (store 8 into `i32** undef`)
+    BRC 15, 4, %bb.2, implicit killed $cc
+  
+  bb.1:
+    liveins: $f2s, $r0d
+  
+    renamable $f0s = COPY killed renamable $f2s
+  
+  bb.2:
+    liveins: $f0s, $r0d
+  
+    STE killed renamable $f0s, undef renamable $r1d, 0, $noreg :: (volatile store 4 into `float* undef`)
+    renamable $r1d = nuw LA %stack.0, 16, $noreg
+    renamable $r2d = nuw LA %stack.0, 24, $noreg
+    renamable $r3d = LA %stack.0, 40, $noreg
+    renamable $r4d = LARL @g_1390
+    STG renamable $r4d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    renamable $r5d = nuw LA %stack.0, 48, $noreg
+    renamable $r14d = LA %stack.0, 72, $noreg
+    renamable $r13d = LA %stack.0, 80, $noreg
+    renamable $r12d = LA %stack.0, 56, $noreg
+    renamable $r10d = LA %stack.0, 0, $noreg
+    STG renamable $r10d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    renamable $r9d = LA %stack.0, 64, $noreg
+    renamable $r8d = LA %stack.0, 88, $noreg
+    renamable $r7d = nuw LA %stack.0, 8, $noreg
+    MVGHI %stack.1, 904, 0
+    STG killed renamable $r9d, $noreg, 0, $noreg :: (store 8 into `i64*** null`)
+    STG killed renamable $r3d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r14d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r7d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r1d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r4d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r2d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r5d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r8d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r12d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r13d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r10d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    $r2l = LHI 0
+    STG killed renamable $r0d, undef renamable $r1d, 0, $noreg :: (store 8 into `i32** undef`)
+    Return implicit $r2l
+
+...
+---
+name:            fun1
+alignment:       16
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r6d' }
+frameInfo:
+  maxAlignment:    8
+stack:
+  - { id: 0, size: 96, alignment: 8 }
+  - { id: 1, size: 1960, alignment: 8 }
+  - { id: 2, size: 8, alignment: 8 }
+  - { id: 3, size: 320, alignment: 8 }
+  - { id: 4, size: 4, alignment: 4 }
+  - { id: 5, size: 8, alignment: 8 }
+  - { id: 6, size: 8, alignment: 8 }
+  - { id: 7, size: 4, alignment: 4 }
+  - { id: 8, size: 8, alignment: 8 }
+  - { id: 9, size: 4, alignment: 4 }
+  - { id: 10, size: 8, alignment: 8 }
+  - { id: 11, size: 8, alignment: 8 }
+  - { id: 12, size: 8, alignment: 8 }
+  - { id: 13, size: 8, alignment: 8 }
+  - { id: 14, size: 24, alignment: 4 }
+  - { id: 15, size: 4, alignment: 4 }
+  - { id: 16, size: 1792, alignment: 8 }
+  - { id: 17, size: 8, alignment: 8 }
+  - { id: 18, size: 8, alignment: 8 }
+  - { id: 19, size: 8, alignment: 8 }
+  - { id: 20, size: 1, alignment: 2 }
+  - { id: 21, size: 672, alignment: 8 }
+  - { id: 22, size: 4, alignment: 4 }
+  - { id: 23, size: 4, alignment: 4 }
+  - { id: 24, size: 4, alignment: 4 }
+  - { id: 25, size: 64, alignment: 8 }
+machineFunctionInfo: {}
+body:             |
+  bb.0:
+    liveins: $r6d
+  
+    STG killed renamable $r6d, undef renamable $r1d, 0, $noreg :: (store 8 into `i32** undef`)
+    renamable $r0d = LARL @g_181
+    STG renamable $r0d, undef renamable $r1d, 0, $noreg :: (store 8 into `i32** undef`)
+    renamable $r1d = nuw LA %stack.0, 16, $noreg
+    renamable $r2d = nuw LA %stack.0, 24, $noreg
+    renamable $r3d = LA %stack.0, 40, $noreg
+    renamable $r4d = LARL @g_1390
+    STG renamable $r4d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    renamable $r5d = nuw LA %stack.0, 48, $noreg
+    renamable $r14d = LA %stack.0, 72, $noreg
+    renamable $r13d = LA %stack.0, 80, $noreg
+    renamable $r12d = LA %stack.0, 56, $noreg
+    renamable $r10d = LA %stack.0, 0, $noreg
+    STG renamable $r10d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    renamable $r9d = LA %stack.0, 64, $noreg
+    renamable $r8d = LA %stack.0, 88, $noreg
+    renamable $r7d = nuw LA %stack.0, 8, $noreg
+    MVGHI %stack.1, 904, 0
+    STG killed renamable $r9d, $noreg, 0, $noreg :: (store 8 into `i64*** null`)
+    STG killed renamable $r3d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r14d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r7d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r1d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r4d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r2d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r5d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r8d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r12d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r13d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    STG killed renamable $r10d, undef renamable $r1d, 0, $noreg :: (store 8 into `i64*** undef`)
+    $r2l = LHI 0
+    STG killed renamable $r0d, undef renamable $r1d, 0, $noreg :: (store 8 into `i32** undef`)
+    Return implicit $r2l
+
+...