[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:50
 #include <iterator>
+#include <set>
 #include <utility>
----------------
It looks like `std::set` isn't used in the patch anywhere, so you probably don't need this.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:52
 #include <utility>
+#include <vector>
 
----------------
You shouldn't have to include `vector` here. It is already included by something else.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5869
+      RepeatedSequenceLocs[0].getMF()->getFrameInstructions();
+  std::vector<MCCFIInstruction> CFIInstructionsPresent;
+
----------------
Can this be a `SmallVector`?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5871-5872
+
+  // We check to see if CFI Instructions are present, and if they are
+  // we add it to a vector
+  bool HasCFI = false;
----------------
I think it would be good to express the intent of this loop here, rather than in the comment below.

e.g.

```
// To safely outline CFI instructions, we must outline every CFI instruction in the function.
// If CFI instructions are present in the candidate, store them in a vector. This can be used to check if we have collected all of the CFI instructions.
```


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5886-5889
+  // Since each set of instructions is a substring, if there are CFI
+  // Instructions and size matches, we know that we have captured all the
+  // CFI instruction, so it will be safe to outline.  If it is not, we do not
+  // return the candidate
----------------
paquette wrote:
> I think we also have to check that the other `Candidate`'s functions have the same number of CFI instructions as well.
> 
> e.g. we shouldn't have
> 
> ```
> foo:
>   cfi 0
>   cfi 1
>   cfi 2
>   ...
>   ret
> 
> bar:
>   cfi 1
>   cfi 2
>   ...
>   ret
> ```
> 
> be outlined to
> 
> ```
> foo:
>   cfi 0
>   b outlined
> 
> bar:
>   b outlined
> ```
> 
> I think that this could happen if for whatever reason `bar` ends up containing the first Candidate.
I think comment is kind of confusing.

- Substring doesn't really imply equality. It would make more sense to say "every candidate has equivalent instructions".

- "size matches" doesn't way what is being referred to with respect to size. You could say "the number of CFI instructions found is equal to the number in the containing function's frame" or something similar.

- I think we should add some reasoning for why we don't want to allow candidates which do not include all of the CFIs.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:5886-5891
+  // Since each set of instructions is a substring, if there are CFI
+  // Instructions and size matches, we know that we have captured all the
+  // CFI instruction, so it will be safe to outline.  If it is not, we do not
+  // return the candidate
+  if (HasCFI && CFIInstructionsPresent.size() != CFIInstructions.size())
+    return outliner::OutlinedFunction();
----------------
I think we also have to check that the other `Candidate`'s functions have the same number of CFI instructions as well.

e.g. we shouldn't have

```
foo:
  cfi 0
  cfi 1
  cfi 2
  ...
  ret

bar:
  cfi 1
  cfi 2
  ...
  ret
```

be outlined to

```
foo:
  cfi 0
  b outlined

bar:
  b outlined
```

I think that this could happen if for whatever reason `bar` ends up containing the first Candidate.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:6194-6196
+  // We only outline these if we will tail call the outlined function, or
+  // fix up the CFI offsets. CFI instructions are outlined only if in a tail
+  // call.
----------------
We don't fix up the offsets, so maybe we should just remove the first sentence in this comment.


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

https://reviews.llvm.org/D77852