[PATCH] D144907: [RegAllocFast] insert additional spills along indirect edges of INLINEASM_BR

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/RegAllocFast.cpp:952-963
+      if (MI.getOpcode() == TargetOpcode::INLINEASM_BR) {
+        int FI = StackSlotForVirtReg[VirtReg];
+        const TargetRegisterClass &RC = *MRI->getRegClass(VirtReg);
+        for (MachineBasicBlock *Succ : MI.getParent()->successors()) {
+          if (Succ->isInlineAsmBrIndirectTarget()) {
+            TII->storeRegToStackSlot(*Succ, Succ->begin(), PhysReg, Kill,
+                FI, &RC, TRI, VirtReg);
----------------
MatzeB wrote:
> nickdesaulniers wrote:
> > MatzeB wrote:
> > > MatzeB wrote:
> > > > MatzeB wrote:
> > > > > May be good to add a comment here to remind readers that we have the really unusual behavior of possibly jumping in the middle of a basic block.
> > > > Would it be possible to move this code block into the `spill()` function. To me it feels like it's just part of the spilling process. You could add a new parameter to pass along `MI` or a `bool MayJump`...
> > > or given there is only this use here, rename to `spillAfter()`, add the `MI` parameter and drop the `SpillBefore` parameter instead. And then just always compute `SpillBefore` within the function based on MI...
> > This works:
> > ```
> > diff --git a/llvm/lib/CodeGen/RegAllocFast.cpp b/llvm/lib/CodeGen/RegAllocFast.cpp
> > index bf99b4b4a0ea..b2ef32a2328c 100644
> > --- a/llvm/lib/CodeGen/RegAllocFast.cpp
> > +++ b/llvm/lib/CodeGen/RegAllocFast.cpp
> > @@ -435,6 +435,7 @@ void RegAllocFast::spill(MachineBasicBlock::iterator Before, Register VirtReg,
> >    LLVM_DEBUG(dbgs() << " to stack slot #" << FI << '\n');
> >  
> >    const TargetRegisterClass &RC = *MRI->getRegClass(VirtReg);
> > +  MachineBasicBlock *MBB = Before->getParent();// shadow MBB member.
> >    TII->storeRegToStackSlot(*MBB, Before, AssignedReg, Kill, FI, &RC, TRI,
> >                             VirtReg);
> >    ++NumStores;
> > @@ -954,9 +955,7 @@ void RegAllocFast::defineVirtReg(MachineInstr &MI, unsigned OpNum,
> >          const TargetRegisterClass &RC = *MRI->getRegClass(VirtReg);
> >          for (MachineBasicBlock *Succ : MI.getParent()->successors()) {
> >            if (Succ->isInlineAsmBrIndirectTarget()) {
> > -            TII->storeRegToStackSlot(*Succ, Succ->begin(), PhysReg, Kill,
> > -                FI, &RC, TRI, VirtReg);
> > -            ++NumStores;
> > +            spill(Succ->begin(), VirtReg, PhysReg, Kill, LRI->LiveOut);
> >              Succ->addLiveIn(PhysReg);
> >            }
> >          }
> > ```
> Calling into `spill()` instead of directly using the `storeRegToStackSlot` callback seems sensible. 
>  Still logically it seems somewhat unfortunate to have part of the spilling logic here and not in the `spill()` function...
We want to repeat the entirety of `spill` but for multiple destinations when encountering an `INLINEASM_BR`.

I could outline the body of `RegAllocFast::spill` into a new method (`RegAllocFast::spillTo` or something), and have `RegAllocFast::spill` check for the special case of `INLINEASM_BR`, calling `RegAllocFast::spillTo` 1-to-many times.

Perhaps it's clearer that `RegAllocFast::defineVirtReg` calls `RegAllocFast::spill` multiple times, rather than one call to `RegAllocFast::spill` resulting in 1-to-many spills?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144907