[PATCH] D77852: Outline CFI Instructions in Tail Calls

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5867
+  // we add it to a vector. We loop over the instructions in each candidate and
+  // colllect CFI instructions to compare against the CFI Instructions in the
+  // original function's frame.  We must check this since if we outline one of
----------------
s/colllect/collect/


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5871
+  // correctness.  If we do not, the address offsets will be incorrect between
+  // the two sections of the program
+  bool HasCFI = false;
----------------
Missing period at the end of sentence. (https://llvm.org/docs/CodingStandards.html#commenting)


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5873
+  bool HasCFI = false;
+  for (outliner::Candidate C : RepeatedSequenceLocs) {
+    std::vector<MCCFIInstruction> CFIInstructions =
----------------
Use `outliner::Candidate &C`?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5876
+        C.getMF()->getFrameInstructions();
+    SmallVector<MCCFIInstruction, 4> CFIInstructionsPresent;
+    MachineBasicBlock::iterator Mit = C.front();
----------------
Pull vector out of the loop and use `clear()`? (See: http://llvm.org/docs/ProgrammersManual.html#vector)


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5878-5884
+    for (unsigned Loc = C.getStartIdx(); Loc < C.getEndIdx() + 1; Loc++) {
+      if (Mit->isCFIInstruction()) {
+        HasCFI = true;
+        unsigned CFIIndex = Mit->getOperand(0).getCFIIndex();
+        MCCFIInstruction CFI = CFIInstructions[CFIIndex];
+        CFIInstructionsPresent.push_back(CFI);
+      }
----------------
I don't think you have to do this for every candidate.

The part you have to do for every candidate is checking the number of CFI instructions in each function frame. This should be sufficient, because we know every instruction in every candidate is the same.

E.g. after collecting the CFI instructions in the first candidate, you can do something like this:

```
if (HasCFI) {
  for (outliner::Candidate &C : RepeatedSequenceLocs) {
    if (C.getMF()->getFrameInstructions().size() != CFIInstructionsPresent.size())
      return outliner::OutlinedFunction();
  }
}
```

or even

```
if (HasCFI && any_of(RepeatedSequenceLocs, ...))
  return outliner::OutlinedFunction()
```

Actually, is it even necessary to //collect// the CFI instructions? Since all we need to know is that the candidates actually contain all of them, it would be sufficient to just count how many are in the candidate versus how many are in the frame of each function, no?


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

https://reviews.llvm.org/D77852