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

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee 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();
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126930