[PATCH] D116424: [ShrinkWrap] check for PPC's non-callee-saved LR

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked 2 inline comments as done.
nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:296
+      // want to shrinkwrap across such an MachineInstr.
+      if (!UseOrDefCSR && MO.isEarlyClobber()) {
+        const MachineFunction *MF = MI.getParent()->getParent();
----------------
jyknight wrote:
> nickdesaulniers wrote:
> > nickdesaulniers wrote:
> > > jyknight wrote:
> > > > nickdesaulniers wrote:
> > > > > FWIW: this special case is also implicit-def. The full MachineOperand is `implicit-def early-clobber $lr`.
> > > > It's not clear to me why this checks "earlyclobber" -- surely it'd be wrong to shrinkwrap around any other uses of LR, as well, not just ones from inlineasm clobbers?
> > > > 
> > > > Thus, I'd suggest removing " && MO.isEarlyClobber()" here. Hopefully that won't cause any more test diffs. (And if it does, either that's showing examples of other more non-asm things we're getting wrong, or else show that I don't understand how it's supposed to work...)
> > > My motivation was that this was probably tied to `TargetOpcode::INLINEASM` and `TargetOpcode::INLINEASM_BR`.  I suspect (but don't know) that they are the only `MachineInstr` that can have `earlyclobber` `MachineOperands`.  That may not be correct; maybe it would better match my intent to just check for those 2 opcodes.
> > > 
> > > That said, if I removed the additional condition on `MO.isEarlyClobber()`, the following tests regress under llvm/test/:
> > > ```
> > > Failed Tests (6):
> > >   LLVM :: CodeGen/PowerPC/ppc-shrink-wrapping.ll
> > >   LLVM :: CodeGen/PowerPC/ppc64-rop-protection-aix.ll
> > >   LLVM :: CodeGen/PowerPC/ppc64-rop-protection.ll
> > >   LLVM :: CodeGen/PowerPC/pr43527.ll
> > >   LLVM :: CodeGen/PowerPC/remove-redundant-load-imm.ll
> > >   LLVM :: CodeGen/PowerPC/xray-conditional-return.ll
> > > ```
> > > (these were ones I had touched in an earlier version of this CL).  Playing with llvm/utils/updated_llc_test_checks.py on some of those, it looks like we're blocking shrinkwrap with that approach).
> > ah, because `$lr` is the operand for each return on ppc: `BLR8 implicit $lr8, implicit $rm`.  Technically, it is a "use" but now we're preventing the restore point from being raised (when I remove the condition on `MO.isEarlyClobber()`).
> > 
> > aarch64 (technically I think it's "ARMv8" and newer) has `ret` instructions; I'm guessing armv7's `bx lr` would also have this issue. Looks like in MIR, `BX_RET 14, $noreg, implicit $r0` is lowered to `bx lr`, so in MIR the equivalent of `bx lr` doesn't list `$lr` as an operand (this is specific to arm). Should those instructions have implicit uses of their link register? Or should ppc's `BLR8` not have an implicit use of `$lr`?
> > 
> > Perhaps changing the condition to `MO.isDef()` (from `MO.isEarlyClobber()`) or `MO.isImp()` would be more appropriate? Or only checking the `MI.getOpcode() == TargetOpcode::INLINEASM || MI.getOpcode() == TargetOpcode::INLINEASM_BR`?
> Maybe excluding MI.isReturn() from the check?
hah! I was literally testing that same idea! Yeah, it works. Uploaded that version, PTAL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116424