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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
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:
> efriedma wrote:
> > 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.
> > Probably the size of CFIInstructions is greater than or equal to the number of CFI instructions in the function 
> 
> I think this is conservatively safe to use it in this context if that happens -- we're likely to bail-out outlining due to mis-matches in # of CFI instructions. I wonder how many cases we have, in practice.
> 
> > Any heuristic that depends on it will be very confusing to anyone looking at MIR dumps (since we print CFI instructions inline).
> 
> Can you elaborate it? Probably I don't understand the problem clearly. Let say we have N as CFIInstructions' size while there are actually M CFI instructions in the function body (M <= N). Didn't M instances still have valid CFI Index, assigned to the function? Do you have an example to show this is the problem?
See the changed llvm/test/CodeGen/AArch64/machine-outliner-cfi-tail.mir ; without this change, we don't transform.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126930