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

[Nick Desaulniers via Phabricator via llvm-commits]

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:
> > 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116424