[PATCH] D116424: [PowerPC] add LR to CalleeSavedRegs
Nick Desaulniers via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 5 12:24:07 PST 2022
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
More information about the llvm-commits
mailing list