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

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee 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));
----------------
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);
    }
```


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:7018
+  for (auto &I : make_range(RepeatedSequenceLocs[0].front(),
+                            std::next(RepeatedSequenceLocs[0].back()))) {
+    if (I.isCFIInstruction())
----------------
I think this is a nice catch. We should check the entire instructions of the candidate which will be outlined, instead of looking the range of index to the hashes.


================
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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126930