This is an archive of the discontinued LLVM Phabricator instance.

[llvm-dwarfutil] Remove unnecessarily dependency.
ClosedPublic

Authored by avl on Aug 4 2022, 5:08 AM.

Details

Summary

llvm-dwarutil.cpp unnecessarily call to InitializeAllAsmParsers.
That call might be removed as well as dependency on ${LLVM_TARGETS_TO_BUILD}

Diff Detail

Event Timeline

avl created this revision.Aug 4 2022, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 5:08 AM
Herald added a subscriber: mgorny. · View Herald Transcript
avl requested review of this revision.Aug 4 2022, 5:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2022, 5:08 AM
avl retitled this revision from [llvm-dwarfutil] Remove redundant depentency. to [llvm-dwarfutil] Remove redundant dependency..Aug 4 2022, 5:10 AM
thakis added a comment.Aug 4 2022, 6:10 AM

Thanks! I'm not an expert in this area, so the below might be silly questions. Apologies in advance :)

llvm/tools/llvm-dwarfutil/CMakeLists.txt
6

Given that you call InitializeAllTargets(), shouldn't you keep this one and instead remove the 3 AllTargetsFoo at the bottom instead?

(InitializeAllTargets calls LLVMInitialize.*Target\b and that's in llvm/lib/Target/Foo/FooTargetMachine.cpp)

llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp
484

It's not obvious to me why this call is redundant, can you elaborate?

avl added inline comments.Aug 4 2022, 3:09 PM
llvm/tools/llvm-dwarfutil/CMakeLists.txt
6

${LLVM_TARGETS_TO_BUILD} includes wider set of dependencies.
If tool does not need f.e. disassembler then we can remove corresponding dependency.
In this concrete case FooDisassembler, FooAsmParser are not neccessary.

If 3 AllTargetsFoo would be removed and ${LLVM_TARGETS_TO_BUILD} will be left then dependency on AllTargetsDisassemblers and others would be added.

llvm/tools/llvm-dwarfutil/llvm-dwarfutil.cpp
484

AsmParser functionality is not used by this tool. Thus we do not need to initialize them.

thakis accepted this revision.Aug 5 2022, 12:43 PM

s/redundantly/unnecessarily/ in commit description then, maybe?

This revision is now accepted and ready to land.Aug 5 2022, 12:43 PM
avl retitled this revision from [llvm-dwarfutil] Remove redundant dependency. to [llvm-dwarfutil] Remove unnecessarily dependency..Aug 8 2022, 5:09 AM
thakis added a comment.Aug 8 2022, 6:18 AM

I meant "redundantly calls" -> "unnecessarily calls".

In the title it should be "Remove unnecessary dependency", without "-ly" :)

This revision was landed with ongoing or failed builds.Aug 8 2022, 7:13 AM
This revision was automatically updated to reflect the committed changes.
avl edited the summary of this revision. (Show Details)Aug 8 2022, 7:14 AM

@thakis Thank you for the review!