[PATCH] D125536: [MC] [MCWin64EH] Try writing an ARM64 "packed epilog" even if the epilog doesn't share opcodes with the prolog

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: llvm/lib/MC/MCWin64EH.cpp:656
 
   info->EpilogMap.clear();
   return Offset;
----------------
efriedma wrote:
> This call to `EpilogMap.clear()` tripped me up in trying to understand the reasoning behind the rest of the changes in this patch.  There are three cases here:
> 
> 1. Unpacked epilogues
> 2. Packed epilogue reusing the prologue
> 3. Packed epilogue not reusing the prologue.
> 
> In the "Packed epilogue reusing the prologue" case, we clear EpilogMap; in the other cases, we don't.
> 
> Maybe the logic here could be a bit more explicit?  Like maybe instead of clearing EpilogMap(), add checks in the caller in the two places it matters.
Yep - this tripped up myself too, when trying to grasp how the existing state worked. In the original setup with only cases 1 and 2, this was a neat (but obscure) requiring very few modifications, but with all these three cases, making it more explicit probably is good.

But then again, given the code below for sharing code between epilogs (where we do `EpilogInstrs.clear()` if the epilog matched another one, in order not to emit it), I think clearing it here if we shouldn't emit it, is fairly consistent. Do you see any other alternative as clearer? We could do `if (PackedEpilogOffset < || PackedEpilogOffset >= PrologCodeBytes) { // emit epilog opcodes }` but I'm not sure if that's any clearer.

I guess I could add a comment here though, e.g. `// Remove the epilog, as we shouldn't emit the epilog opcodes`, and we could consider making it `Epilog.clear()` too (so we keep the epilogmap entry, but remove the opcodes). That'd probably be the most consistent?


================
Comment at: llvm/lib/MC/MCWin64EH.cpp:990
     MCSymbol* MatchingEpilog =
       FindMatchingEpilog(EpilogInstrs, AddedEpilogs, info);
     if (MatchingEpilog) {
----------------
efriedma wrote:
> Possible followup optimization: we could teach FindMatchingEpilog to look at the prologue.  (Not sure how frequently that would trigger.)
Indeed, and I guess that's probably quite common - a function with two epilogs, both which match the prolog perfectly? I'll see if I can try to implement that too as a followup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125536