[PATCH] D116424: [ShrinkWrap] don't sink Save points past INLINEASM_BR MachineInstrs

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/ShrinkWrap.cpp:507-511
+      if (MI.getOpcode() == TargetOpcode::INLINEASM_BR) {
+        LLVM_DEBUG(dbgs() << "inlineasm_br prevents shrink-wrapping\n");
+        return false;
+      }
       if (!useOrDefCSROrFI(MI, RS.get()))
----------------
nickdesaulniers wrote:
> efriedma wrote:
> > efriedma wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > There's something simpler going on here.  The same exact input works as expected for other architectures with link registers, such as `arm` and `aarch64`. It seems that `useOrDefCSROrFI` is returning `true` for the `INLINEASM_BR` for those architectures, but not `powerpc64le`.
> > > > In `aarch64` and `arm`, the `MachineOperand` `useOrDefCSROrFI` is interested is marked `dead`, but for `ppc64le` it's not.
> > > > 
> > > > `implicit-def dead early-clobber $lr` (arm, aarch64)
> > > > `implicit-def early-clobber $lr` (ppc64le)
> > > > 
> > > > For `arm` and `aarch64`, the `MachineOperand` is marked dead by the `livevars` pass. For ppc64le, it does not.
> > > > 
> > > > ---
> > > > 
> > > > Also, within `useOrDefCSROrFI`, the call to `RCI.getLastCalleeSavedAlias` returns 0 on ppc, unlike aarch64 and arm.
> > > The PowerPC "lr" register is not a general-purpose register, and can only be accessed by a few specialized instructions.  Because of this, it isn't marked allocatable (which is why it isn't marked "dead").  Not sure this is actually the right decision; if we want various interactions here to work well, it probably should be allocatable.
> > > 
> > > That said, given the current state, the best incremental step might be something like isNonallocatableRegisterCalleeSave.
> > > 
> > > I assume this issue applies to both INLINEASM and INLINEASM_BR.
> > > That said, given the current state, the best incremental step might be something like isNonallocatableRegisterCalleeSave.
> > 
> > 
> > I mean, adding a new target hook named something like isNonallocatableRegisterCalleeSave
> Shouldn't `LR` be listed explicitly in llvm/lib/Target/PowerPC/PPCCallingConv.td `def CSR_PPC64`'s list of `CalleeSavedRegs`?  `LR` is explicitly listed in llvm/lib/Target/AArch64/AArch64CallingConvention.td `def CSR_AArch64_AAPCS`'s `CalleeSavedRegs`.
```
diff --git a/llvm/lib/Target/PowerPC/PPCCallingConv.td b/llvm/lib/Target/PowerPC/PPCCallingConv.td
index 1e81276f1de3..f9162a3716fe 100644
--- a/llvm/lib/Target/PowerPC/PPCCallingConv.td
+++ b/llvm/lib/Target/PowerPC/PPCCallingConv.td
@@ -298,7 +298,7 @@ def CSR_PPC64   : CalleeSavedRegs<(add X14, X15, X16, X17, X18, X19, X20,
                                         X21, X22, X23, X24, X25, X26, X27, X28,
                                         X29, X30, X31, F14, F15, F16, F17, F18,
                                         F19, F20, F21, F22, F23, F24, F25, F26,
-                                        F27, F28, F29, F30, F31, CR2, CR3, CR4
+                                        F27, F28, F29, F30, F31, CR2, CR3, CR4, LR
                                    )>;
```
appears to fix shrinkwrap, but later asserts:
```
Unknown regclass!
```
If I use `LR8` instead, then shrink-wrap doesn't work; I think `implicit-def early-clobber $lr` `MachineOperand` might need to be emitted using `$lr8`? ie. `implicit-def early-clobber $lr8`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116424