[SystemZ, IPRA] determineCalleeSaves must always add return register and DP.

[jonpa]

Summary

When enabling IPRA on SystemZ, a benchmark crashed and I found out that a function had been optimized to not save the Callee Saved Registers, which unfortunately included the return register (%r14) on SystemZ, so when the function tried to return UB resulted.

To fix this, I beleive SystemZFrameLowering::determineCalleeSaves() needs to add R14D to SavedRegs. I also think the DP should be added (R11), in case the calling function uses DP, right?

In SystemZFrameLowering::determineCalleeSaves(), the first call to TargetFrameLowering::determineCalleeSaves(MF, SavedRegs, RS) will not add any registers to SavedRegs if

// When interprocedural register allocation is enabled caller saved registers
// are preferred over callee saved registers.
if (MF.getTarget().Options.EnableIPRA && isSafeForNoCSROpt(MF.getFunction()))
  return;

So, the question then is what registers we must save in this case, and which we in fact do not have to save, to get better performance with IPRA. So far, I have come to the conclusion that all the specially treated registers that SystemZFrameLowering::determineCalleeSaves is already adding needs to be saved with IPRA.

I made two tests - one each for R11 and R14. For some reason I could not process the debug output without doing a separate run for it.

[vivekvpandya]

When enabling IPRA on SystemZ, a benchmark crashed and I found out that a function had been optimized to not save the Callee Saved Registers, which unfortunately included the return register (%r14) on SystemZ, so when the function tried to return UB resulted.

Why the caller function did not save/restore return register value around when for a callee IPRA decided to have noCSR opt?
Also I think there should be other architecture for which also these tests will break due to same reason. If this is bug applicable for other architectures too then I think it is better to update TargetFrameLowering::determineCalleeSaves() so that it can add such specially treated registers to saved list or other architecture must update their determineCalleeSaves() as implemented in this change.

To fix this, I beleive SystemZFrameLowering::determineCalleeSaves() needs to add R14D to SavedRegs. I also think the DP should be added (R11), in case the calling function uses DP, right?

I see determineCalleeSaves() implementation for few other architectures and none of them checking if IPRA is enable so are they handling such case in different way or they are unaware of this kind of failure?

[jonpa]

In D46232#1082240, @vivekvpandya wrote:

When enabling IPRA on SystemZ, a benchmark crashed and I found out that a function had been optimized to not save the Callee Saved Registers, which unfortunately included the return register (%r14) on SystemZ, so when the function tried to return UB resulted.

Why the caller function did not save/restore return register value around when for a callee IPRA decided to have noCSR opt?

The caller makes a call with 'brasl %r14, fun', which sets %r14 to the return address. When the callee returns, it does 'br %r14'. So it is not around the call to 'fun' the return register needs to be saved/restored, but *inside* of it.

Also I think there should be other architecture for which also these tests will break due to same reason. If this is bug applicable for other architectures too then I think it is better to update TargetFrameLowering::determineCalleeSaves() so that it can add such specially treated registers to saved list or other architecture must update their determineCalleeSaves() as implemented in this change.

To fix this, I beleive SystemZFrameLowering::determineCalleeSaves() needs to add R14D to SavedRegs. I also think the DP should be added (R11), in case the calling function uses DP, right?

I see determineCalleeSaves() implementation for few other architectures and none of them checking if IPRA is enable so are they handling such case in different way or they are unaware of this kind of failure?

That's a good point. I suspect the other targets have to do the same thing, since determineCalleeSaves is there to specify this. I mean it is not just the return register, but also for instance on SystemZ some landing pad registers.

[uweigand]

I do not believe we need to do anything special with R11. This is only special if it is used as frame pointer, in which case the code before already adds it as caller-saved:

if (HasFP)
  SavedRegs.set(SystemZ::R11D);

I agree we need to handle R14, however. This is for the scenario where we have a leaf function (so the MFFrame.hasCalls() check will return false), but R14 is still clobbered by some *other* instruction in the current function. In this case we always need to save and restore it, since the return instruction will implicitly rely on the incoming R14 value.

[vivekvpandya]

lib/Target/SystemZ/SystemZFrameLowering.cpp

97

From last comment by @uweigand it seems that the actual bug is here shouldn't hasCall() for systemZ detect instructions which modifies R14 except load instruction?

[uweigand]

Well, usually this is handled correctly by the isPhysRegModified call in SystemZFrameLowering::determineCalleeSaves. (The only reason why we even need the extra hasCalls check is that the call instruction is currently not at all stages modeled correctly to show that R14 is clobbered.)

The only problem is that in the IPRA special case, the SystemZFrameLowering::determineCalleeSaves handling is skipped. I think we simply should do an unconditional check in SystemZ code, like so:

if (MFFrame.hasCalls() || MRI.isPhysRegModified(SystemZ::R14D))
  SavedRegs.set(SystemZ::R14D);

[jonpa]

I do not believe we need to do anything special with r11. This is only special if it is used as frame pointer, in which case the code before already adds it as caller-saved:

if (HasFP)
SavedRegs.set(SystemZ::R11D);

I reran ipra-03.ll with saving r11 commented out, and got this debug output:

++++++++++++++++++++ Register Usage Information Propagation ++++++++++++++++++++
MachineFunction : fun0
 -------------------- Register Usage Information Collector Pass --------------------
Function Name : fun0
Clobbered Registers: fun0 function optimized for not having CSR.
$cc $r0d $r1d $r2d $r3d $r11d $r12d $r13d $r14d $r15d $r1h $r2h $r13h $r14h $r15h $r0l $r1l $r2l $r3l $r11l $r12l $r13l $r14l $r15l $r0q $r2q $r10q $r12q $r14q
----------------------------------------
 ++++++++++++++++++++ Register Usage Information Propagation ++++++++++++++++++++
MachineFunction : BZ2_blockSort
Call Instruction Before Register Usage Info Propagation :
CallBRASL @fun0, $r2d, $r3d, $r4d, $r5d, $r6d, <regmask $f8d $f9d $f10d $f11d $f12d $f13d $f14d $f15d $f8q $f9q $f12q $f13q $f8s $f9s $f10s $f11s $f12s $f13s $f14s $f15s $r6d $r7d $r8d $r9d \
$r10d $r11d $r12d $r13d $r14d $r15d $r6h $r7h $r8h $r9h $r10h $r11h $r12h $r13h $r14h $r15h $r6l $r7l $r8l $r9l $r10l $r11l $r12l $r13l $r14l $r15l $r6q $r8q $r10q $r12q $r14q>, implicit-def\
 dead $r14d, implicit-def dead $cc, implicit-def $r2d

Call Instruction After Register Usage Info Propagation : CallBRASL @fun0, $r2d, $r3d, $r4d, $r5d, $r6d, <regmask $noreg $a0 $a1 $a2 $a3 $a4 $a5 $a6 $a7 $a8 $a9 $a10 $a11 $a12 $a13 $a14 $a15 \
$c0 $c1 $c2 $c3 $c4 $c5 $c6 $c7 $c8 $c9 $c10 $c11 $c12 $c13 $c14 $c15 $v0 $v1 $v2 $v3 $v4 $v5 $v6 $v7 $v8 $v9 $v10 $v11 $v12 $v13 $v14 $v15 $v16 $v17 $v18 $v19 $v20 $v21 $v22 $v23 $v24 $v25 \
$v26 $v27 $v28 $v29 $v30 $v31 $f0d $f1d $f2d $f3d $f4d $f5d $f6d $f7d $f8d $f9d $f10d $f11d $f12d $f13d $f14d $f15d $f16d $f17d $f18d $f19d $f20d $f21d $f22d $f23d $f24d $f25d $f26d $f27d $f\
28d $f29d $f30d $f31d $f0q $f1q $f4q $f5q $f8q $f9q $f12q $f13q $f0s $f1s $f2s $f3s $f4s $f5s $f6s $f7s $f8s $f9s $f10s $f11s $f12s $f13s $f14s $f15s $f16s $f17s $f18s $f19s $f20s $f21s $f22\
s $f23s $f24s $f25s $f26s $f27s $f28s $f29s $f30s $f31s $r4d $r5d $r6d $r7d $r8d $r9d $r10d $r0h $r3h $r4h $r5h $r6h $r7h $r8h $r9h $r10h $r11h $r12h $r4l $r5l $r6l $r7l $r8l $r9l $r10l $r4q\
 $r6q $r8q>, implicit-def dead $r14d, implicit-def dead $cc, implicit-def $r2d

fun0 uses %r11l, so Collector adds %r11l and %r11d. Propagation then updates the regmask on the call to fun0 to clobber those registers.

I am a bit unsure about what would happen with %r11 in the calling function if hasFP() returns true. In that case %r11 is a reserved register in that function, so regalloc will not try to use it. In the top of Propagate, I see

/// This updated RegMask will be used by the register allocator while allocating
/// the current MachineFunction.

It doesn't say here that it will save a reserved register around a call that clobbers it. Generally, I would fear that a reserved register is under the responsibility of the Target, but maybe I am missing something here? (I assume that's why we have to add it in determineCalleeSaves ourselves if HasFP is true?)

[uweigand]

This looks like a generic problem to me, many platforms have registers that are reserved only in some functions but not others. If function A where a register is reserved calls function B where the register is not reserved, something must ensure that the register is preserved across the call to B. Usually, this is the case because such registers need to be callee-saved. But if function B is optimized via IPRA to not save the register, we have a problem ...

Can you check how this is solved on other platforms? I guess X86 likewise should use a conditionally-reserved register as frame pointer ...

[vivekvpandya]

I run ipra-02.ll with --enable-ipra option with lldb the contorl reaches to SystemZFrameLowering::determineCalleeSaves() through https://github.com/llvm-mirror/llvm/blob/5b508199fa870516e29b847b8af2f85887e05d56/lib/CodeGen/PrologEpilogInserter.cpp#L525
for both functions HasFP is set to false that is why determineCalleeSaves() does not add R11. Can you please verify this?

[vivekvpandya]

test/CodeGen/SystemZ/ipra-03.ll

88

here changing "no-frame-pointer-elim"="true" fixes the problem. It adds spill/restore for R11

[uweigand]

But we do not want to force use of a frame pointer, in general this just reduces performance for no reason on Z. Some functions require a frame pointer (those that do dynamic allocas), but most do not.

[vivekvpandya]

In D46232#1083113, @uweigand wrote:

But we do not want to force use of a frame pointer, in general this just reduces performance for no reason on Z. Some functions require a frame pointer (those that do dynamic allocas), but most do not.

Then I think code should be modified in following manner:

if (HasFP || MRI.isPhysRegModified(SystemZ::R11D) )
  SavedRegs.set(SystemZ::R11D);

[uweigand]

So, in effect, never do the IPRA callee-saved optimization for R11, because the caller *could* use it as frame pointer? That's certainly the conservative solution.

In fact, we're then bascially back to Jonas' original patch anyway.

lib/Target/SystemZ/SystemZFrameLowering.cpp

void SystemZFrameLowering::determineCalleeSaves(MachineFunction &MF,
 
   // If the function requires a frame pointer, record that the hard
   // frame pointer will be clobbered.
   if (HasFP)
     SavedRegs.set(SystemZ::R11D);
 
   // If the function calls other functions, record that the return
   // address register will be clobbered.
   if (MFFrame.hasCalls())
     SavedRegs.set(SystemZ::R14D);
 
+  // With interprocedural register allocation, saving the CSRs may be skipped
+  // by TargetFrameLowering if it is considered safe. Make sure in that case
+  // that the FP and return registers are always restored if clobbered.
+  if (MF.getTarget().Options.EnableIPRA) {
+    const MachineRegisterInfo &MRI = MF.getRegInfo();
+    if (MRI.isPhysRegModified(SystemZ::R11D))
+      SavedRegs.set(SystemZ::R11D);
+    if (MRI.isPhysRegModified(SystemZ::R14D))
+      SavedRegs.set(SystemZ::R14D);
+  }
+
   // If we are saving GPRs other than the stack pointer, we might as well
   // save and restore the stack pointer at the same time, via STMG and LMG.
   // This allows the deallocation to be done by the LMG, rather than needing
   // a separate %r15 addition.
   const MCPhysReg *CSRegs = TRI->getCalleeSavedRegs(&MF);
   for (unsigned I = 0; CSRegs[I]; ++I) {
     unsigned Reg = CSRegs[I];
     if (SystemZ::GR64BitRegClass.contains(Reg) && SavedRegs.test(Reg)) {

test/CodeGen/SystemZ/ipra-02.ll

+; Test that the return register is saved and restored even when the saving of
+; Callee Saved Registers is skipped with interprocedural register allocation.
+;
+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -enable-ipra < %s \
+; RUN:   | FileCheck  %s
+;
+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -enable-ipra \
+; RUN:   -debug-only=ip-regalloc 2>&1 < %s | FileCheck --check-prefix=DBG %s
+;
+
+; DBG: fun3 function optimized for not having CSR
+; CHECK-LABEL: fun3
+; CHECK: stmg    %r14, %r15, 112(%r15)
+; CHECK: lr      %r14
+; CHECK: a       %r14
+; CHECK: lmg     %r14, %r15, 112(%r15)
+; CHECK: br      %r14
+
+
+@board = external dso_local local_unnamed_addr global [421 x i8], align 2
+@delta = external dso_local local_unnamed_addr global [8 x i32], align 4
+
+; Function Attrs: nounwind
+define internal fastcc signext i32 @fun0(i32 signext, i32*, i32 signext, i32 signext) unnamed_addr #0 {
+  br i1 undef, label %5, label %6
+
+; <label>:5:                                      ; preds = %4
+  unreachable
+
+; <label>:6:                                      ; preds = %4
+  switch i32 undef, label %10 [
+    i32 1, label %7
+    i32 2, label %8
+    i32 3, label %9
+  ]
+
+; <label>:7:                                      ; preds = %6
+  unreachable
+
+; <label>:8:                                      ; preds = %6
+  unreachable
+
+; <label>:9:                                      ; preds = %6
+  unreachable
+
+; <label>:10:                                     ; preds = %6
+  %11 = call fastcc signext i32 @fun2(i32 signext undef, i32* nonnull undef, i32 signext %2, i32 signext %3)
+  unreachable
+}
+
+; Function Attrs: nounwind
+define dso_local signext i32 @find_defense(i32 signext, i32*) local_unnamed_addr #0 {
+  %3 = call fastcc signext i32 @fun1(i32 signext %0, i32* nonnull undef, i32 signext 0, i32 signext 0)
+  ret i32 undef
+}
+
+; Function Attrs: nounwind
+define internal fastcc signext i32 @fun1(i32 signext, i32*, i32 signext, i32 signext) unnamed_addr #0 {
+  switch i32 undef, label %8 [
+    i32 1, label %5
+    i32 2, label %7
+  ]
+
+; <label>:5:                                      ; preds = %4
+  %6 = call fastcc signext i32 @fun0(i32 signext undef, i32* null, i32 signext undef, i32 signext undef) #2
+  unreachable
+
+; <label>:7:                                      ; preds = %4
+  unreachable
+
+; <label>:8:                                      ; preds = %4
+  unreachable
+}
+
+; Function Attrs: nounwind
+define internal fastcc signext i32 @fun2(i32 signext, i32*, i32 signext, i32 signext) unnamed_addr #0 {
+  %5 = call fastcc signext i32 @fun3(i32 signext %0, i32 signext undef, i32* nonnull undef)
+  unreachable
+}
+
+; Function Attrs: norecurse nounwind
+define internal fastcc signext i32 @fun3(i32 signext, i32 signext, i32* nocapture) unnamed_addr #1 {
+  %4 = load i8, i8* undef, align 1
+  %5 = getelementptr inbounds [8 x i32], [8 x i32]* @delta, i64 0, i64 0
+  %6 = load i32, i32* %5, align 4
+  %7 = load i32, i32* undef, align 4
+  %8 = add nsw i32 %6, %1
+  br label %9
+
+; <label>:9:                                      ; preds = %3
+  %10 = load i8, i8* undef, align 1
+  %11 = icmp eq i8 %10, 0
+  br i1 %11, label %12, label %15
+
+; <label>:12:                                     ; preds = %9
+  %13 = icmp eq i8 undef, %4
+  %14 = or i1 undef, %13
+  br label %17
+
+; <label>:15:                                     ; preds = %9
+  %16 = sub nsw i32 0, %7
+  br label %17
+
+; <label>:17:                                     ; preds = %15, %12
+  %18 = phi i32 [ %16, %15 ], [ %7, %12 ]
+  br label %19
+
+; <label>:19:                                     ; preds = %17
+  %20 = shl nsw i32 %18, 1
+  %21 = add nsw i32 %20, %8
+  %22 = sext i32 %21 to i64
+  %23 = getelementptr inbounds [421 x i8], [421 x i8]* @board, i64 0, i64 %22
+  %24 = load i8, i8* %23, align 1
+  %25 = icmp eq i8 %24, 3
+  br i1 %25, label %28, label %26
+
+; <label>:26:                                     ; preds = %19
+  %27 = add nsw i32 %18, %1
+  store i32 %27, i32* %2, align 4
+  unreachable
+
+; <label>:28:                                     ; preds = %19
+  ret i32 0
+}
+
+attributes #0 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="z13" "target-features"="+transactional-execution,+vector" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { norecurse nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="z13" "target-features"="+transactional-execution,+vector" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = { nounwind }

test/CodeGen/SystemZ/ipra-03.ll

+; Test that the DP (%r11) register is saved and restored even when the saving of
+; Callee Saved Registers is skipped with interprocedural register allocation.
+;
+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -enable-ipra < %s \
+; RUN:   | FileCheck  %s
+;
+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=z13 -enable-ipra \
+; RUN:   -debug-only=ip-regalloc 2>&1 < %s | FileCheck --check-prefix=DBG %s
+;
+
+; DBG: fun0 function optimized for not having CSR
+; CHECK-LABEL: fun0
+; CHECK: stmg    %r11, %r15
+; CHECK: llc     %r11
+; CHECK: lmg     %r11, %r15
+; CHECK: br      %r14
+
+%0 = type { i32, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, i8*, %1*, %0*, i32, i32, i64, i16, i8, [1 x i8], i8*, i64, i8*, i8*, i8*, i8*, i64, i32, [20 x i8] }
+%1 = type { %1*, %0*, i32 }
+%2 = type { %3*, i32, i32, i32, i32*, i32*, i32*, i32, i32*, i8*, i16*, i8*, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, [256 x i8], [256 x i8], i32, i32, i32, i32, i32, i32, i32, i32, [258 x i32], [18002 x i8], [18002 x i8], [6 x [258 x i8]], [6 x [258 x i32]], [6 x [258 x i32]], [258 x [4 x i32]] }
+%3 = type { i8*, i32, i32, i32, i8*, i32, i32, i32, i8*, i8* (i8*, i32, i32)*, void (i8*, i8*)*, i8* }
+
+; Function Attrs: nounwind
+define dso_local void @BZ2_blockSort(%2* nocapture) local_unnamed_addr {
+  br label %2
+
+; <label>:2:                                      ; preds = %1
+  br label %3
+
+; <label>:3:                                      ; preds = %4, %2
+  br label %4
+
+; <label>:4:                                      ; preds = %3
+  br i1 undef, label %3, label %5
+
+; <label>:5:                                      ; preds = %4
+  %6 = call fastcc zeroext i8 @fun0(i32 zeroext undef, i32 zeroext undef, i8* undef, i16* undef, i32 zeroext undef, i32* nonnull undef)
+  unreachable
+}
+
+; Function Attrs: norecurse nounwind
+define internal fastcc zeroext i8 @fun0(i32 zeroext, i32 zeroext, i8* readonly, i16* nocapture readonly, i32 zeroext, i32* nocapture) unnamed_addr #4 {
+  br label %7
+
+; <label>:7:                                      ; preds = %29, %6
+  %8 = phi i32 [ undef, %6 ], [ %33, %29 ]
+  %9 = phi i32 [ undef, %6 ], [ %32, %29 ]
+  %10 = phi i32 [ undef, %6 ], [ %34, %29 ]
+  br label %11
+
+; <label>:11:                                     ; preds = %7
+  %12 = add i32 %9, 6
+  %13 = add i32 %8, 6
+  %14 = zext i32 %12 to i64
+  %15 = getelementptr inbounds i8, i8* %2, i64 %14
+  %16 = load i8, i8* %15, align 1
+  %17 = zext i32 %13 to i64
+  %18 = getelementptr inbounds i8, i8* %2, i64 %17
+  %19 = load i8, i8* %18, align 1
+  %20 = icmp eq i8 %16, %19
+  br i1 %20, label %22, label %21
+
+; <label>:21:                                     ; preds = %11
+  ret i8 undef
+
+; <label>:22:                                     ; preds = %11
+  %23 = getelementptr inbounds i16, i16* %3, i64 %14
+  %24 = load i16, i16* %23, align 2
+  %25 = getelementptr inbounds i16, i16* %3, i64 %17
+  %26 = load i16, i16* %25, align 2
+  %27 = icmp eq i16 %24, %26
+  br i1 %27, label %29, label %28
+
+; <label>:28:                                     ; preds = %22
+  unreachable
+
+; <label>:29:                                     ; preds = %22
+  %30 = add i32 %8, 8
+  %31 = select i1 undef, i32 0, i32 %4
+  %32 = sub i32 0, %31
+  %33 = sub i32 %30, 0
+  %34 = add nsw i32 %10, -8
+  store i32 0, i32* %5, align 4
+  %35 = icmp sgt i32 %10, 7
+  br label %7
+}
+
+attributes #4 = { norecurse nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="z13" "target-features"="+transactional-execution,+vector" "unsafe-fp-math"="false" "use-soft-float"="false" }
+