[PATCH] D83923: [MachineOutliner][AArch64] Fix for noreturn functions

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:6110
     std::vector<outliner::Candidate> CandidatesWithoutStackFixups;
+    std::vector<outliner::Candidate> CandidatesWithMultiplePotentialStackFixups;
 
----------------
Why are we saving these instead of the candidates that we could potentially outline?

This part of the code is mostly intended to decide on whether or not we should discard candidates which require stack fixups. (It should probably be factored out into a function to make that clearer, but that's another patch for another day.)

I think that you probably want to drop these candidates from the main list specifically.

I think something like this should work (using the nice range-based `erase_if` and `any_of` from STLExtras.h)

```
  if (FlagsSetInAll & MachineOutlinerMBBFlags::HasCalls) {
    // Remove all candidates which would require multiple stack fixups.
    erase_if(RepeatedSequenceLocs, [](Candidate &C) {
      if (C.LRU.available(AArch64::LR) && !IsNoReturn)
        return false;
      return any_of(C, [](const MachineInstr &MI) { return MI.isCall(); });
    });

    if (RepeatedSequenceLocs.size() < 2) {
      // We removed enough candidates that outlining would no longer be
      // beneficial. Bail out.
      RepeatedSequenceLocs.clear();
      return outliner::OutlinedFunction();
    }
  }
```


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:6127-6130
+      // If the current containing function of this candidate is a noreturn
+      // function and the candidate itself contains calls, then the resulting
+      // call to the outlined function from this candidate will be a call from a
+      // noreturn function to a function that contains one or more calls.
----------------
Maybe an example would be useful here, rather than trying to explain everything in words:

```
// Fixing up the stack twice is not yet implemented.
// 
// E.g. cases like this are not yet supported:
//
// function_containing_sequence:
//   ...
//   save LR to SP <- Requires stack instr fixups in OUTLINED_FUNCTION_N
//   call OUTLINED_FUNCTION_N
//   restore LR from SP
//   ...
//
// OUTLINED_FUNCTION_N:
//   save LR to SP <- Requires stack instr fixups in OUTLINED_FUNCTION_N
//   ...
//   bl foo
//   restore LR from SP
//   ret
//
// This happens whenever function_containing_sequence
// - Is noreturn
// - Requires that we save LR at the point of the call, but there are no
//   available registers. In this case, we save using SP.
//
// TODO: Check if fixing up twice is safe so we can outline these.
```


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:6143
+      //
+      // The assert happens in this file with the same Bugzilla ID referenced.
+      //
----------------
Which file?


================
Comment at: llvm/lib/Target/AArch64/AArch64InstrInfo.cpp:6187-6190
+    if (CandidatesWithMultiplePotentialStackFixups.size()) {
+      RepeatedSequenceLocs.clear();
+      return outliner::OutlinedFunction();
+    }
----------------
I think what we really want to do here is remove the candidates which require multiple fixups from the list.

Let's say we have something like this:

```
some_noreturn_func:
  ...
  candidate_sequence_containing_call
  ...

dog:
  ...
  candidate_sequence_containing_call
  ...

cat:
  ...
  candidate_sequence_containing_call
  ...

fish:
  ...
  candidate_sequence_containing_call
  ...
```

The call imposes one round of stack fixups, since we save LR using SP inside the function frame.

Let's say that in `dog`, `cat`, and `fish`, we don't require fixups at the callsite.

For example, we could outline from `dog` like this:

```
dog:
  ...
  bl OUTLINED_FUNCTION_N
  ...
```

This is fine; we fix up the stack instructions once inside `OUTLINED_FUNCTION_N`.

This patch bails out when we have a situation like this. If you removed the candidate that appeared in `some_noreturn_func` from the list, however, you'd be able to outline from `dog`, `cat`, and `fish` safely.

So what I'd expect to end up with is, say, something like this:

```
some_noreturn_func:
  ...
  candidate_sequence_containing_call <-- Don't outline, noreturn means we need to fix up at the callsite.
  ...

dog:
  ...
  bl OUTLINED_FUNCTION_N
  ...

cat:
  ...
  save LR to a register <-- This is fine, it does not impose any stack fixups
  bl OUTLINED_FUNCTION_N
  restore LR from a register
  ...

fish:
  ...
  bl OUTLINED_FUNCTION_N
  ...

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83923