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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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`.
That might work?  Might need some additional adjustments.  I don't think any other targets put reserved/non-allocatable registers in the CalleeSavedRegs list.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116424