[PATCH] D56813: [AArch64] [Windows] Share unwind codes between epilogues with identical unwind codes

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

It might be nice to merge together identical suffixes eventually, but that shouldn't block this change.

How hard would it be to add support for reusing opcodes from an identical prolog?  The unwind format is designed to allow that in some cases.



================
Comment at: lib/MC/MCWin64EH.cpp:459
+// Epilogs - Epilogs that potentialy match the current epilog.
+static std::pair<bool, MCSymbol*>
+FindMatchingEpilog(const std::vector<WinEH::Instruction>& EpilogInstrs,
----------------
Using a pair here seems weird... either use llvm:Optional, or just return a null pointer.


================
Comment at: lib/MC/MCWin64EH.cpp:468
+    const auto &Instrs = InstrsIter->second;
+    unsigned NumMatchingInstrs = 0;
+
----------------
A bool would be sufficient here.


================
Comment at: lib/MC/MCWin64EH.cpp:513
+  // size of the unwind codes.
+  DenseMap<uint32_t, DenseMap<uint32_t, std::vector<MCSymbol *>>> AddedEpilogs;
+
----------------
The DenseMaps here seem unnecessary; I mean, yes, it'll reduce the number of potential matching epilogs, but realistically functions have very few unique epilogs anyway.


================
Comment at: lib/MC/MCWin64EH.cpp:529
+      EpilogInfo[EpilogStart] = EpilogInfo[MatchingEpilog.second];
+      EpilogInstrs.clear();
+    } else {
----------------
It's not immediately obvious that you're mutating the EpilogMap here; probably worth calling that out explicitly in a comment.  I guess it does the right thing because nothing else will use the FrameInfo after this.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56813