[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);
----------------
nickdesaulniers wrote:
> 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?
Actually, I might be able to make `RegAllocFast::spill` recursive...let me give that a shot.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144907