[PATCH] D126930: Fix interaction of CFI instructions with MachineOutliner.

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:672
       MCCFIInstruction CFI = Instrs[CFIIndex];
-      (void)MF.addFrameInst(CFI);
+      NewMI->removeOperand(0);
+      MachineInstrBuilder(MF, NewMI).addCFIIndex(MF.addFrameInst(CFI));
----------------
kyulee wrote:
> I think it's somewhat inefficient by cloning the entire instruction, removing this and then adding one back.
> CFIIndex should be reassigned for the new function (because the old function might optimize or shuffle it), so we will create new CFI instruction while adding CFIIndex to the new function. Can we just do this way?
> ```
>     // Don't keep debug information for outlined instructions.
>     auto DL = DebugLoc();
>     if (I->isCFIInstruction()) {
>       unsigned CFIIndex = I->getOperand(0).getCFIIndex();
>       MCCFIInstruction CFI = Instrs[CFIIndex];
>       BuildMI(MBB, MBB.end(), DL, TII.get(TargetOpcode::CFI_INSTRUCTION))
>            .addCFIIndex(MF.addFrameInst(CFI));
>     } else {
>       MachineInstr *NewMI = MF.CloneMachineInstr(&*I);
>       NewMI->dropMemRefs(MF);
>       NewMI->setDebugLoc(DL);
>       MBB.insert(MBB.end(), NewMI);
>     }
> ```
The efficiency difference is unlikely to matter, but sure, this is probably more clear.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:7030
+    for (outliner::Candidate &C : RepeatedSequenceLocs) {
+      if (std::next(C.getMF()->begin()) != C.getMF()->end())
+        return outliner::OutlinedFunction();
----------------
kyulee wrote:
> I'm just a bit confused about this code. Are you checking a function with a single block?
> Even that, I don't think walking all the instruction in the block adds value -- do you see more outlining or improvement by doing this?
> The existing logic that just checks the size of CFIInstructions might be enough conservatively although it might include dead CFIs.
> If you see the following logic later, the machine outliner actually outlines a tail case only if CFIs are outlined.
Probably the size of CFIInstructions is greater than or equal to the number of CFI instructions in the function (unless we clone a CFI instruction, which probably shouldn't happen).

That said, I don't really want to trust the size of CFIInstructions.  Any heuristic that depends on it will be very confusing to anyone looking at MIR dumps (since we print CFI instructions inline).

The single basic block restriction isn't strictly necessary; I'll try to come up with a better way to write this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126930