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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB 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:
> > 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...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144907