[PATCH] D116424: [PowerPC] add LR to CalleeSavedRegs

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

In D116424#3223110 <https://reviews.llvm.org/D116424#3223110>, @efriedma wrote:

> In D116424#3223071 <https://reviews.llvm.org/D116424#3223071>, @nickdesaulniers wrote:
>
>> In D116424#3222971 <https://reviews.llvm.org/D116424#3222971>, @efriedma wrote:
>>
>>> From the test changes, it looks like the current patch completely disables shrink-wrapping in all cases?  Or am I missing something?
>>
>> Doesn't `@loopInfoRestoreOutsideLoop` in llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll show that that's NOT the case? The `mflr`/`mtlr` pair are sunk+raised respectively since the entry block is conditional and doesn't use the stack.
>>
>> `@foo` in llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll also has an early return rather than an unconditional prolog. `bgelr` (branch to lr if greater than or equal to). Compared to the codegen with shrink wrap disabled, the `mflr`/`mtlr` pair is unconditional.
>>
>> Though I'm less convinced about `@@shrinkwrap` in llvm/test/CodeGen/PowerPC/ppc64-rop-protection.ll and `@test` in llvm/test/CodeGen/PowerPC/pr43527.ll.
>
> Maybe verify that llvm/test/CodeGen/PowerPC/ppc-shrink-wrapping.ll shrink-wraps in both 32-bit and 64-bit modes?
>
> In any case, the patch is clearly having some effect beside the one you're trying for.



  diff --git a/llvm/lib/CodeGen/ShrinkWrap.cpp b/llvm/lib/CodeGen/ShrinkWrap.cpp
  index f89069e9f728..01ec62dc02a0 100644
  --- a/llvm/lib/CodeGen/ShrinkWrap.cpp
  +++ b/llvm/lib/CodeGen/ShrinkWrap.cpp
  @@ -288,8 +288,13 @@ bool ShrinkWrap::useOrDefCSROrFI(const MachineInstr &MI,
         // separately. An SP mentioned by a call instruction, we can ignore,
         // though, as it's harmless and we do not want to effectively disable tail
         // calls by forcing the restore point to post-dominate them.
  +      const MachineFunction *MF = MI.getParent()->getParent();
  +      const TargetRegisterInfo *TRI = MF->getSubtarget().getRegisterInfo();
         UseOrDefCSR = (!MI.isCall() && PhysReg == SP) ||
  -                    RCI.getLastCalleeSavedAlias(PhysReg);
  +                    RCI.getLastCalleeSavedAlias(PhysReg) ||
  +                    (MO.isEarlyClobber() &&
  +                     !TRI->isCalleeSavedPhysReg(PhysReg, *MF) &&
  +                     !TRI->isInAllocatableClass(PhysReg));
       } else if (MO.isRegMask()) {
         // Check if this regmask clobbers any of the CSRs.
         for (unsigned Reg : getCurrentCSRs(RS)) {

fixes just the intended case of `ClobberLR_BR` in llvm/test/CodeGen/PowerPC/ppc64-inlineasm-clobber.ll without regressing any other tests and without any other changes necessary.

It's written assuming that early-clobber is only used in clobber lists of `TargetOpcode::INLINEASM` and `TargetOpcode::INLINEASM_BR`. IDK if that's actually the case (ie. can other `MachineInstr`s have early-clobber `MachineOperands`?); perhaps I should check for either of those two opcodes instead?  The second part of the check for ` !TRI->isCalleeSavedPhysReg(PhysReg, *MF) && !TRI->isInAllocatableClass(PhysReg))` could instead be converted into a `virtual` method on `TargetRegisterInfo` called `isNonallocatableRegisterCalleeSave` that returns false, and is overridden by ` PPCRegisterInfo ` to return `true` for `LR` and `LR8` parameters?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116424/new/

https://reviews.llvm.org/D116424