This is an archive of the discontinued LLVM Phabricator instance.

Cleanup header dependencies in LLVMCore
ClosedPublic

Authored by serge-sans-paille on Jan 31 2022, 1:42 PM.

Details

Summary

Based on the output of include-what-you-use.

This is a big chunk of changes. It is very likely to break downstream code
unless they took a lot of care in avoiding hidden ehader dependencies, something
the LLVM codebase doesn't do that well :-/

I've tried to summarize the biggest change below:

  • llvm/include/llvm-c/Core.h: no longer includes llvm-c/ErrorHandling.h
  • llvm/IR/DIBuilder.h no longer includes llvm/IR/DebugInfo.h
  • llvm/IR/IRBuilder.h no longer includes llvm/IR/IntrinsicInst.h
  • llvm/IR/LLVMRemarkStreamer.h no longer includes llvm/Support/ToolOutputFile.h
  • llvm/IR/LegacyPassManager.h no longer include llvm/Pass.h
  • llvm/IR/Type.h no longer includes llvm/ADT/SmallPtrSet.h
  • llvm/IR/PassManager.h no longer includes llvm/Pass.h nor llvm/Support/Debug.h

And the usual count of preprocessed lines:
$ clang++ -E -Iinclude -I../llvm/include ../llvm/lib/IR/*.cpp -std=c++14 -fno-rtti -fno-exceptions | wc -l
before: 6400831
after: 6189948

200k lines less to process is no that bad ;-)

Discourse thread on the topic: https://llvm.discourse.group/t/include-what-you-use-include-cleanup

Diff Detail

Event Timeline

serge-sans-paille requested review of this revision.Jan 31 2022, 1:42 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 31 2022, 1:42 PM

Thanks for working on this. I spot checked some files. In general it looks great.

One file replaces a forward declaration with #include "llvm/IR/BasicBlock.h".
Some files add #include "llvm/Support/ToolOutputFile.h".
One files adds class FunctionPass;

Would you mind landing these addition changes separately to make the include removal part more targeted?

MaskRay added a comment.EditedJan 31 2022, 2:05 PM

It is very likely to break downstream code unless they took a lot of care in avoiding hidden ehader dependencies, something the LLVM codebase doesn't do that well :-/

release/14.x will branch soon. Will it be useful to deliberately postpone the header removal part after the branch point so that downstream code (with some folks testing trunk llvm) has more time to fix or will that cause headache when llvm-project developers back port fixes to release/14.x?

I'm not sure how it'd help clients of the released version of LLVM to delay, the development branch and the release seems fairly disconnected to me from this point of view.
(what can help are deprecating APIs with instructions about how to update so that they can update to the new API while targeting the current release)

Hi and thanks for taking the time to go through the review.

Thanks for working on this. I spot checked some files. In general it looks great.

One file replaces a forward declaration with #include "llvm/IR/BasicBlock.h".

Yeah. What happens is that header was previously implicitly included (and needed), I just made the include explicit (and the forward declaration useless)

Some files add #include "llvm/Support/ToolOutputFile.h".

Yeah, that's a consequence of llvm/IR/LLVMRemarkStreamer.h no longer includes llvm/Support/ToolOutputFile.h

One files adds class FunctionPass;

Yeah, that happens when the file was implicitly including llvm/Pass.h but was actually only needing a forward declaration.

Would you mind landing these addition changes separately to make the include removal part more targeted?

The three changes above are tied to individual header removal in specific header. I could split that in smaller grain (say each header change + its impact on the codebase in individual commits), but that would mean that every "component" cleanup would consist in potentially dozens of commits. I understand the value of such fine-grain commits, but it means more care (and work!) on my side to handle more reviews, craft more decent commit message etc. I chose to split the cleanup per component to make review easier, have a decent idea of the impact on preprocessed lines. I'm trying to reflect in the commit message the major changes to help downstream users update their codebase.

If you really think the split would help the review process, I can do it, but please take into account the amount of work it adds on an already not super-creative task ;-)

I'm not sure how it'd help clients of the released version of LLVM to delay, the development branch and the release seems fairly disconnected to me from this point of view.
(what can help are deprecating APIs with instructions about how to update so that they can update to the new API while targeting the current release)

I second that opinion.

ormris removed a subscriber: ormris.Feb 1 2022, 9:47 AM
MaskRay accepted this revision.Feb 1 2022, 10:44 AM

Thanks!

This revision is now accepted and ready to land.Feb 1 2022, 10:44 AM
This revision was landed with ongoing or failed builds.Feb 1 2022, 9:56 PM
This revision was automatically updated to reflect the committed changes.