This is an archive of the discontinued LLVM Phabricator instance.

[AtomicExpandPass] Always copy pcsections Metadata to expanded atomics
ClosedPublic

Authored by melver on Aug 1 2022, 1:58 AM.

Details

Summary

When expanding IR atomics to target-specific atomics, copy all
!pcsections Metadata to expanded atomics automatically.

Depends on D130884

Diff Detail

Event Timeline

melver created this revision.Aug 1 2022, 1:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 1:58 AM
melver requested review of this revision.Aug 1 2022, 1:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 1:59 AM
melver updated this revision to Diff 450030.Aug 4 2022, 9:58 AM
melver edited the summary of this revision. (Show Details)

Rebase.

I guess this patch can be tested with regual IR test?

melver updated this revision to Diff 457542.Sep 2 2022, 2:48 AM

Add IR test.

PTAL.

vitalybuka accepted this revision.Sep 2 2022, 10:24 AM
vitalybuka added inline comments.
llvm/test/Transforms/AtomicExpand/AArch64/pcsections.ll
4309

I don't know how stable the order
often in such generated test I had to replace -NEXT with -DAG

9eadad51bafb741203e21d917a43d45c99fabe55
and
c0ee9e1b98cac2cc4b3369dbcf52486e4d294060
When it was -NEXT it failed bunch of bots, but I can't reproduce locally even with LLVM_REVERSE_ITERATION or LLVM_ENABLE_EXPENSIVE_CHECKS (I just noticed, that i didn't try gcc, which is used on those bots)

This revision is now accepted and ready to land.Sep 2 2022, 10:24 AM
arsenm added inline comments.Sep 9 2022, 7:10 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
130–133

This seems too specific. Why do we need a new IRBuilder subclass just for this? Isn't there already an existing helper for metadata? We could also preserve a lot more metadata, like any aliasing info

melver added inline comments.Sep 12 2022, 10:14 AM
llvm/lib/CodeGen/AtomicExpandPass.cpp
130–133

There's no existing helper (except CollectMetadataToCopy()) AFAIK.

The alternative is to explicitly add a Builder.CollectMetadataToCopy everywhere, but I felt that this is more concise given it's done everywhere.

This subclass is only available to AtomicExpandPass. I tried to have a local BuildIRBuilder() function, but IR builder doesn't like being copied. Having a subclass that does something extra in the constructor seemed just as elegant and simple.