This is an archive of the discontinued LLVM Phabricator instance.

[DWARFLinkerParallel] Add support of accelerator tables to DWARFLinkerParallel.
ClosedPublic

Authored by avl on Jul 9 2023, 10:17 AM.

Details

Summary

This patch is extracted from D96035, it adds support for the accelerator
tables to the DWARFLinkerParallel functionality.

run-time performance and memory requirements for clang binary --num-threads 16 :

----------------------------------------------------------------------------------
                                                      |  time, sec   |  mem, GB  |
----------------------------------------------------------------------------------
dsymutil --no-odr --accelerator Apple --linker llvm   |      55      |   20.0    |
----------------------------------------------------------------------------------
dsymutil --no-odr --accelerator Apple --linker apple  |     263      |   23.0    |
----------------------------------------------------------------------------------

run-time performance and memory requirements for clang binary --num-threads 1 :

----------------------------------------------------------------------------------
                                                      |  time, sec   |  mem, GB  |
----------------------------------------------------------------------------------
dsymutil --no-odr --accelerator Apple --linker llvm   |     273      |   19.0    |
----------------------------------------------------------------------------------
dsymutil --no-odr --accelerator Apple --linker apple  |     276      |   21.0    |
----------------------------------------------------------------------------------

Depends on D153268

Diff Detail

Event Timeline

avl created this revision.Jul 9 2023, 10:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2023, 10:17 AM
avl requested review of this revision.Jul 9 2023, 10:17 AM
Herald added a project: Restricted Project. · View Herald Transcript
avl edited the summary of this revision. (Show Details)Jul 9 2023, 10:18 AM
avl updated this revision to Diff 546430.Aug 2 2023, 6:06 AM

rebased.

avl updated this revision to Diff 552311.Aug 22 2023, 4:22 AM

rebased.

avl updated this revision to Diff 556417.Sep 11 2023, 4:00 AM

rebased.

avl added a comment.Sep 11 2023, 4:01 AM

friendly ping.

Let some minor comments, @JDevlieghere should probably take a look, too.

llvm/include/llvm/CodeGen/AccelTable.h
320

Document what the variant cases mean?

llvm/lib/DWARFLinkerParallel/ArrayList.h
52

If (!CurGroup->Next)

61–62

for (auto &item: CurGroup)

Handler(Item)
llvm/lib/DWARFLinkerParallel/DWARFLinkerCompileUnit.cpp
1434

Can this loop indefinitely if the input DWARF has circular references?

avl updated this revision to Diff 556490.Sep 11 2023, 2:55 PM

addressed comments(documented CUs parameter, replaced check for nullptr, used foreach loop, added check for infinite loop in case bad DWARF).

avl added a comment.Sep 18 2023, 2:21 AM

friendly ping.

avl updated this revision to Diff 557106.Sep 20 2023, 5:23 AM

rebased.

I haven't paid much attention to phab since the github transition. This is a pretty big patch, I'll do a first pass today.

JDevlieghere added inline comments.Sep 21 2023, 12:04 PM
llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
422–425

Very cool

llvm/lib/DWARFLinkerParallel/DIEAttributeCloner.cpp
229

Maybe add an inline comment /*CanResolveInterCUReferences=*/ or we can use an enum like this:

enum ResolveInterCUReferences : bool {
  CanResolveInterCUReferences = true,
  CannotResolveInterCUReferences = false,
};
llvm/lib/DWARFLinkerParallel/DIEAttributeCloner.h
23–28

Can we continue to do the initialization inline?

llvm/lib/DWARFLinkerParallel/DWARFLinkerCompileUnit.cpp
1437

10 seems relatively small. What about something like 100 or 1000? Its only purpose is avoiding infinite recursion, right? Might as well pick something that's basically impossible to hit.

1468

FWIW, I don't think we need to keep any dsymutil-classic compatibility in the parallel linker. There's no way it's going to generate bit-identical output so there's no value in keeping bug-for-bug compat.

avl added inline comments.Sep 22 2023, 2:40 AM
llvm/lib/DWARFLinkerParallel/DIEAttributeCloner.h
23–28

After C++20 would be supported :-) AFAIK, The default bitfield member initialization is a C++20 feature.

Probably, we can remove bitfields usages here as in current configuration they do not buy anything.

avl updated this revision to Diff 557241.Sep 22 2023, 7:09 AM

address review comments.

avl updated this revision to Diff 557268.Sep 23 2023, 6:41 AM

rebased.

This revision is now accepted and ready to land.Sep 26 2023, 3:09 PM
avl added a comment.Sep 26 2023, 3:13 PM

Thanks for the review!

MaskRay added inline comments.Sep 26 2023, 3:17 PM
llvm/test/tools/dsymutil/X86/basic-linking-bundle.test
27

cp is better

llvm/test/tools/dsymutil/X86/basic-linking-x86.test
49

cp

llvm/test/tools/dsymutil/X86/multiple-inputs.test
85

2-column indentation for continuation lines

avl updated this revision to Diff 557398.Sep 27 2023, 3:38 AM

addressed comments.