[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:
> 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`?
if I use `LR8` in llvm/lib/Target/PowerPC/PPCCallingConv.td AND `lr8` in the clobber list, then that works... IIUC, `lr8` is the 64b alias of the 32b `lr`? Then shouldn't `lr` in inline asm clobber list be emitted as `LR8` rather than `LR`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116424