[SystemZ] Save/restore r6 and r7 if function contains landing pad.

[• koriakin]

Summary

This fixes PR27102.

[uweigand]

It would be better to get the register numbers via the existing TargetLowering hooks getExceptionPointerRegister and getExceptionSelectorRegister.

This way, it might be possible to move the code to the common code base implementation of determineCalleeSaves in CodeGen/TargetFrameLoweringImpl.cpp.

B.t.w. I'm wondering why this doesn't already work, since common code already knows about those registers and marks them as live-in to the landing pad basic blocks:

BB#2: derived from LLVM BB %lpad, EH LANDING PAD
    Live Ins: %R6D %R7D %R11D
    Predecessors according to CFG: BB#0
        EH_LABEL <MCSym=.Ltmp2>
        STG %R6D<kill>, %R11D, 176, %noreg; mem:ST8[%exn.slot]
        %R0L<def> = COPY %R7L, %R7D<imp-use,kill>
        ST %R0L<kill>, %R11D, 172, %noreg; mem:ST4[%ehselector.slot]
        J <BB#3>

For some reason, this doesn't seem to be enough to consider those registers modified by the MRI.isPhysRegModified hook called from the common code determineCalleeSaves implementation ...

[uweigand]

As far as I can see, the getPersonalities call here operates on a per-module basis, i.e. it returns all personalities used by any function in the current module, not just the current function. If other functions in the module use exceptions, but not this one, this may lead to overindication here. Not sure if that is a problem in practice ...

In any case, since this is now no longer a SystemZ specific fix, it would probably be better to add reviewers more familiar with this part of common code.

[• koriakin]

In D18541#417904, @uweigand wrote:

As far as I can see, the getPersonalities call here operates on a per-module basis, i.e. it returns all personalities used by any function in the current module, not just the current function. If other functions in the module use exceptions, but not this one, this may lead to overindication here. Not sure if that is a problem in practice ...

That was a bad thinko...

[MatzeB]

I think you should rather change the TargetFrameLowering::determineCalleeSaves() callback of the target to mark the registers as callee saved.

If this really makes sense for all targets then it could also be done in the PrologEpilogueInsertion code before/after determineCalleeSaves(), but I am not convinced yet this applies universally (though I'm no expert in exception handling code).

[• koriakin]

In D18541#418973, @MatzeB wrote:

I think you should rather change the TargetFrameLowering::determineCalleeSaves() callback of the target to mark the registers as callee saved.

If this really makes sense for all targets then it could also be done in the PrologEpilogueInsertion code before/after determineCalleeSaves(), but I am not convinced yet this applies universally (though I'm no expert in exception handling code).

Well, this applies to any target whose exception pointer/selector registers are callee-save. I'm not sure if there's any target other than SystemZ where that applies for the default calling convention. However, it can apply to other targets as well if you have a landing pad in a function with eg. preserve_mostcc calling convention - is that supposed to work?

[MatzeB]

In D18541#419066, @koriakin wrote:

In D18541#418973, @MatzeB wrote:

I think you should rather change the TargetFrameLowering::determineCalleeSaves() callback of the target to mark the registers as callee saved.

If this really makes sense for all targets then it could also be done in the PrologEpilogueInsertion code before/after determineCalleeSaves(), but I am not convinced yet this applies universally (though I'm no expert in exception handling code).

Well, this applies to any target whose exception pointer/selector registers are callee-save. I'm not sure if there's any target other than SystemZ where that applies for the default calling convention. However, it can apply to other targets as well if you have a landing pad in a function with eg. preserve_mostcc calling convention - is that supposed to work?

So I've been thinking about this a bit more. We actually have a similar concept already in MachineBasicBlock::getBeginClobberMask() and MachineBasicBlock::getEndClobberMask() that gives you the set of registers clobbered between basic blocks which is exactly used to describe the clobbering effects of the personality function.

My first intuition would be that we just need to provide the proper clobber mask for all personality functions to get your desired behavior. However currently this would not help because these extra clobber masks are not passed to MachineRegisterInfo::addPhysRegsUsedFromRegMask() in the VirtRegRewriter and thus are not considered for prologue epilogue insertion.

Do we actually generate a proper epilogue when we leave the function via an exception? Maybe that is not the case for WinEH which would already be an example against these clobbers needing to be unconditionally spilled/restored...

+CC Reid, David who may have more insight into exception handling ABIs...

[MatzeB]

This was no accident, I very carefully arranged to represent these masks as something other than MachineOperands specifically so that PEI *wouldn't* consider them clobbered. :)

On Win64, the unwinder arranges to restore all your registers to what they were at the point of the throwing call site, either after you rejoin normal control flow (via catchret in LLVM) or by unwinding out of the frame. They *don't* restore the registers on entry into a funclet cleanuppad or catchpad, which is why we have these register masks in in the first place. Win64 has *many* CSRs including XMM registers, so if things were not arranged this way, all functions using WinEH would have ridiculously large prologues.

While in principle this is rather a problem for shrink wrapping and not announcing the clobber to MRI::addPhysRegsUsedMask() is wrong. I can see that in practice we are simply not there yet.

So it seems to me like the best action today would be to leave the decision of what registers modified by the personality function should be saved/restored to the target in TargetFrameLowering::determineCalleeSaves(), and maybe provide a shared implementation of this bit to avoid code duplication...

[• koriakin]

In D18541#419458, @MatzeB wrote:

This was no accident, I very carefully arranged to represent these masks as something other than MachineOperands specifically so that PEI *wouldn't* consider them clobbered. :)

On Win64, the unwinder arranges to restore all your registers to what they were at the point of the throwing call site, either after you rejoin normal control flow (via catchret in LLVM) or by unwinding out of the frame. They *don't* restore the registers on entry into a funclet cleanuppad or catchpad, which is why we have these register masks in in the first place. Win64 has *many* CSRs including XMM registers, so if things were not arranged this way, all functions using WinEH would have ridiculously large prologues.

While in principle this is rather a problem for shrink wrapping and not announcing the clobber to MRI::addPhysRegsUsedMask() is wrong. I can see that in practice we are simply not there yet.

So it seems to me like the best action today would be to leave the decision of what registers modified by the personality function should be saved/restored to the target in TargetFrameLowering::determineCalleeSaves(), and maybe provide a shared implementation of this bit to avoid code duplication...

Well, my first diff was like that (I forgot to add context, but the implementation is indeed in SystemZTargetFrameLowering::determineCalleeSaves()), so I'm fine with that.

But, how about keeping this target-independent, and making it depend on personality function kind instead? AFAICS the underlying issue also affects x86_64-linux with coldcc - do we want to support this?

[• koriakin]

Back to original version (SystemZ only).

[uweigand]

Well, if there's no simple way to do it in common code, then I guess I'm fine with the back-end patch.

LGTM.

lib/CodeGen/MachineRegisterInfo.cpp

 //===-- lib/Codegen/MachineRegisterInfo.cpp -------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // Implementation of the MachineRegisterInfo class.
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/IR/Function.h"
 #include "llvm/Support/raw_os_ostream.h"
 #include "llvm/Target/TargetInstrInfo.h"
+#include "llvm/Target/TargetLowering.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Target/TargetSubtargetInfo.h"
 
 using namespace llvm;
 
 // Pin the vtable to this file.
 void MachineRegisterInfo::Delegate::anchor() {}
 
bool MachineRegisterInfo::isPhysRegModified(unsigned PhysReg) const {
   const TargetRegisterInfo *TRI = getTargetRegisterInfo();
   for (MCRegAliasIterator AI(PhysReg, TRI, true); AI.isValid(); ++AI) {
     for (const MachineOperand &MO : make_range(def_begin(*AI), def_end())) {
       if (isNoReturnDef(MO))
         continue;
       return true;
     }
   }
+  const TargetLowering *TLI = MF->getSubtarget().getTargetLowering();
+  for (const Function *Personality : MF->getMMI().getPersonalities()) {
+    if (TLI->getExceptionPointerRegister(Personality) == PhysReg
+        || TLI->getExceptionSelectorRegister(Personality) == PhysReg)
+      return true;
+  }
   return false;
 }
 
 bool MachineRegisterInfo::isPhysRegUsed(unsigned PhysReg) const {
   if (UsedPhysRegMask.test(PhysReg))
     return true;
   const TargetRegisterInfo *TRI = getTargetRegisterInfo();
   for (MCRegAliasIterator AliasReg(PhysReg, TRI, true); AliasReg.isValid();
        ++AliasReg) {
     if (!reg_nodbg_empty(*AliasReg))
       return true;
   }
   return false;
 }

test/DebugInfo/SystemZ/eh_frame_personality.ll

clean:
   %tst = landingpad { i8*, i32 } cleanup
   ret i64 42
 }
 
 ; CHECK-FUNC: foo:
 ; CHECK-FUNC: .cfi_startproc
 ; CHECK-FUNC: .cfi_personality 0, __gxx_personality_v0
 ; CHECK-FUNC: .cfi_lsda 0, .Lexception0
-; CHECK-FUNC: stmg  %r14, %r15, 112(%r15)
+; CHECK-FUNC: stmg  %r6, %r15, 48(%r15)
+; CHECK-FUNC: .cfi_offset %r6, -112
+; CHECK-FUNC: .cfi_offset %r7, -104
 ; CHECK-FUNC: .cfi_offset %r14, -48
 ; CHECK-FUNC: .cfi_offset %r15, -40
 ; CHECK-FUNC: aghi  %r15, -160
 ; CHECK-FUNC: .cfi_def_cfa_offset 320
 ; ...main function...
 ; CHECK-FUNC: .cfi_endproc
 ;
 ; CHECK-ET: .section  .gcc_except_table,"a",@progbits