This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Async unwind - function epilogues
ClosedPublic

Authored by chill on Oct 22 2021, 9:25 AM.

Diff Detail

Event Timeline

chill created this revision.Oct 22 2021, 9:25 AM
chill requested review of this revision.Oct 22 2021, 9:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2021, 9:25 AM
chill retitled this revision from [AArch64] Async unwind (6/6)- function epilogues to [AArch64] Async unwind (6/6) - function epilogues.Oct 22 2021, 9:25 AM
chill planned changes to this revision.Oct 22 2021, 9:30 AM
chill updated this revision to Diff 389541.Nov 24 2021, 10:26 AM
chill retitled this revision from [AArch64] Async unwind (6/6) - function epilogues to [AArch64] Async unwind - function epilogues.
chill added reviewers: t.p.northover, efriedma, MaskRay.
chill updated this revision to Diff 392400.Dec 7 2021, 7:45 AM
chill added inline comments.Dec 8 2021, 8:14 AM
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1867

Note to self: remove.

chill marked an inline comment as not done.Dec 8 2021, 8:14 AM
chill updated this revision to Diff 399015.Jan 11 2022, 10:39 AM
chill updated this revision to Diff 408914.Feb 15 2022, 9:16 AM

LGTM

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1963–1965

looks it can be removed.

MaskRay accepted this revision.Feb 24 2022, 2:13 PM

LGTM.

This revision is now accepted and ready to land.Feb 24 2022, 2:13 PM
chill updated this revision to Diff 412122.Mar 1 2022, 8:40 AM
chill marked an inline comment as done.
MaskRay accepted this revision.Mar 1 2022, 8:51 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 8:51 PM
This revision was landed with ongoing or failed builds.Mar 2 2022, 5:32 AM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Mar 4 2022, 8:12 AM
chill updated this revision to Diff 413013.Mar 4 2022, 8:14 AM
chill added a comment.Mar 4 2022, 8:28 AM

Committing this change caused failures in the asan/hwasan regression tests. The reason was that emitting unwind info
in epilogues caused the unwind information in case like below:

foo:
  ...
  CSR saves
  ...
  CSR restores
  ret
L:
  ...
  b <somewhere>

to no longer be accidentally correct for the region starting at L. That could be a miscompilation
if a throwing call somehow ends up in that region.

Thus, I've changed the order of the patches to bring CFIFixup before the this patch.

chill requested review of this revision.Mar 4 2022, 8:30 AM
chill updated this revision to Diff 413422.Mar 7 2022, 4:18 AM
chill updated this revision to Diff 417926.Mar 24 2022, 7:32 AM

Updated on top of latest main, will commit it the following days, based on previous
acceptance, as it contains no non-trivial changes.

chill accepted this revision.Mar 24 2022, 7:34 AM
This revision is now accepted and ready to land.Mar 24 2022, 7:34 AM
chill updated this revision to Diff 419189.Mar 30 2022, 9:55 AM
MaskRay accepted this revision.Apr 7 2022, 10:05 PM

Note: arm64-shrink-wrapping.ll and merge-store-dependency.ll need rebase.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.cpp
129–143
if (!NeedsAsyncDwarfUnwindInfo) {
  const Function &F = MF.getFunction();
  // TODO hasMinSize() is to ...
  NeedsAsyncDwarfUnwindInfo = 
}
return *NeedsAsyncDwarfUnwindInfo;

operator* is more common than getValue. The comment is too long to use the in-line style.

llvm/test/CodeGen/AArch64/framelayout-sve.mir
266

Specify the operands of CFI_INSTRUCTION

343

Specify the operands of CFI_INSTRUCTION

499

Specify the operands of CFI_INSTRUCTION

chill added inline comments.Apr 11 2022, 10:29 AM
llvm/test/CodeGen/AArch64/framelayout-sve.mir
266

Looks redundant to me. The operands are evident from the ASM and UNWINDINFO checks.

chill updated this revision to Diff 421969.Apr 11 2022, 10:50 AM
chill marked an inline comment as done.
MaskRay accepted this revision.Apr 11 2022, 12:15 PM
MaskRay added inline comments.
llvm/test/CodeGen/AArch64/framelayout-sve.mir
266

SVE has quite complex code paths in the implementation, so it's good to test at least the CFI instruction name to have some coverage. Other operands may be a bit redundant.

Matt added a subscriber: Matt.Apr 11 2022, 2:20 PM
chill updated this revision to Diff 422218.Apr 12 2022, 6:53 AM
chill marked 4 inline comments as done.
This revision was landed with ongoing or failed builds.Apr 12 2022, 8:53 AM
This revision was automatically updated to reflect the committed changes.