[PATCH] D47655: [MachineOutliner] Don't outline sequences where x16/x17/nzcv are live across

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: include/llvm/CodeGen/MachineOutliner.h:170
+    std::for_each((MachineBasicBlock::reverse_iterator)front(), MBB->rend(),
+                  [this](MachineInstr &MI) { LRUIn.stepBackward(MI); });
+  }
----------------
efriedma wrote:
> This could get expensive with a long basic block; I guess we can try to add some sort of cache later if it becomes an issue.
If it'd be possible to infer candidate liveness from the entire block's liveness, we could wrap the candidate's MBB in a struct like

```
struct CandidateMBB
{
    MachineBasicBlock *MBB;
    LiveRegUnits LRU;
    ...
};
```

Then we wouldn't risk recalculating liveness on one basic block over and over again in the case of, say, overlapping candidates. 


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:4977
+
+    if (!LRUIn.available(AArch64::W17) || !LRUOut.available(AArch64::W17))
+      return true;
----------------
efriedma wrote:
> We don't care if it's live out: we can ignore the ABI rules because we know the exact destination of the "bx lr".
> 
> We could save/restore x16/x17 in theory, instead of giving up, but probably not worth implementing.
> We don't care if it's live out: we can ignore the ABI rules because we know the exact destination of the "bx lr".

Oh, good, much simpler then. :)

> We could save/restore x16/x17 in theory, instead of giving up, but probably not worth implementing.

Yeah, I think most outlined sequences are too short to make saving/restoring x16/x17 worth it. However, if it turned out that just *one* candidate required the save/restore, then we could possibly salvage the candidate and still outline. I haven't checked to see if that's a common case at all though.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:5022
+                       [](outliner::Candidate &C) {
+                         return C.LRUOut.available(AArch64::LR);
+                         })) {
----------------
efriedma wrote:
> I guess there isn't any practical difference between LRUOut and LRUIn here because we don't outline instructions other than calls which read/write LR?
Yep. It's worth a comment explaining that though. LRUOut was effectively what was being used before for LR as well.

(It might be worth it to try allowing instructions that read/write LR in general though, and then discarding candidates that use them if they aren't part of a prospective tail call.)


https://reviews.llvm.org/D47655