This is an archive of the discontinued LLVM Phabricator instance.

[Support] Move ItaniumManglingCanonicalizer and SymbolRemappingReader from Support to ProfileData
ClosedPublic

Authored by RKSimon on Feb 4 2023, 4:20 AM.

Details

Summary

As mentioned on https://discourse.llvm.org/t/issues-in-llvm-tblgen-high-parallelized-build/68037, ItaniumManglingCanonicalizer is often slow to build, resulting in a bottleneck for distributed builds while waiting for LLVMSupport to complete.

SymbolRemappingReader is the only current user of ItaniumManglingCanonicalizer, and this is only used by ProfileData and llvm-cxxmap - so I propose we move both files into the ProfileData library.

An alternative would be to begin a LLVMSymbol library but I don't think we have a strong enough need for that yet.

Diff Detail

Event Timeline

RKSimon created this revision.Feb 4 2023, 4:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2023, 4:20 AM
RKSimon requested review of this revision.Feb 4 2023, 4:20 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptFeb 4 2023, 4:20 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

ItaniumManglingCanonicalizer.cpp doesn't have many includes. I think it is fine to remain in llvm/lib/Support ...

phosek added a subscriber: phosek.Feb 4 2023, 10:26 AM

Another alternative would be to move these classes to the Demangle library.

ItaniumManglingCanonicalizer.cpp doesn't have many includes. I think it is fine to remain in llvm/lib/Support ...

The frontend cost might not be high - but ItaniumManglingCanonicalizer.cpp is still slow to build, so getting it out of something as core as Support still make senses

Another alternative would be to move these classes to the Demangle library.

I'm happy to move this to LLVMDemangle instead if everyone agrees.

@RKSimon
Could you cherry-pick it?
https://github.com/chapuni/llvm-project/commit/2c09237d8759bb6b1e99940e78901bde5b2bb98f

As a position to reduce overhead in tblgen, I don't agree to move this to LLVMDemangle.
LLVMSupport depends on LLVMDemangle. llvm-tblgen depends on them.

Besides, does this depend on LLVMSupport if you would move this to LLVMDemangle?

I think this is one of practical resolutions. One concern, "Does it make sense if llvm-cxxmap depends on LLVMProfile?"

@MaskRay
Fair enough, even if its user is only LLVMProfileData in the trunk.
That said, in fact, ItaniumManglingCanonicalizerTest.cpp spends the most time to build in LLVMSupport.

I don't have a strong opinion for this to move to LLVMProfileData.
Could we introduce another library, like "LLVMMangleSupport" ?

RKSimon updated this revision to Diff 494917.Feb 5 2023, 8:05 AM

Add bazel build fix for llvm-cxxmap

RKSimon updated this revision to Diff 494925.Feb 5 2023, 8:25 AM

fix the git diff lost renames

This (moving to ProfileData) seems like a simple and pragmatic solution for a (perhaps not gigantic but) real problem. It feels fine to me.

MaskRay accepted this revision.Feb 6 2023, 9:28 AM

LGTM.

This revision is now accepted and ready to land.Feb 6 2023, 9:28 AM
This revision was landed with ongoing or failed builds.Feb 6 2023, 12:56 PM
This revision was automatically updated to reflect the committed changes.