This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by mstorsjo on May 13 2022, 3:59 AM.

Details

Summary

The "packed epilog" form only implies that the epilog is located
exactly at the end of the function (so the location of the epilog
is implicit from the epilog opcodes), but it doesn't have to share
opcodes with the prolog - as long as the total number of opcode
bytes and the offset to the epilog fit within the bitfields.

This avoids writing a 4 byte epilog scope in many cases. (I haven't
measured how much this shrinks actual xdata sections in practice
though.)

Diff Detail

Event Timeline

mstorsjo created this revision.May 13 2022, 3:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2022, 3:59 AM
mstorsjo requested review of this revision.May 13 2022, 3:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2022, 3:59 AM
efriedma added inline comments.May 13 2022, 4:34 PM
llvm/lib/MC/MCWin64EH.cpp
656

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.

990

Possible followup optimization: we could teach FindMatchingEpilog to look at the prologue. (Not sure how frequently that would trigger.)

1029

Is the fixme about sharing epilog codes fixed?

mstorsjo added inline comments.May 14 2022, 12:05 PM
llvm/lib/MC/MCWin64EH.cpp
656

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?

990

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.

mstorsjo added inline comments.May 14 2022, 2:50 PM
llvm/lib/MC/MCWin64EH.cpp
1029

Yeah, I think so. (We only share epilogs if they're a perfect match, while we should be able to share them if one is a subset of the other one. But I think the potential benefit from that is rather small.)

mstorsjo updated this revision to Diff 429492.May 14 2022, 4:09 PM

After thinking more, I don't think other ways of handling the original case of packed+shared epilogs really are any clearer, but I added a comment to the existing info->EpilogMap.clear() call at least.

The other change (use packed form to specify the epilog opcode offset) doesn't seem to have almost any effect on the real world size - but this patch clearly has. For a 5.8 MB DLL, this patch shrinks it by 0.7%, shrinking the 412 KB xdata section to 372 KB.

This revision is now accepted and ready to land.May 16 2022, 11:08 AM
This revision was landed with ongoing or failed builds.May 16 2022, 2:41 PM
This revision was automatically updated to reflect the committed changes.