This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Move StringExtras.h include from Error.h to Error.cpp
ClosedPublic

Authored by IncludeGuardian on Jun 18 2023, 10:56 AM.

Details

Summary

Move the implementation of the toString function from llvm/Support/Error.h to the source file,
which allows us to move #include "llvm/ADT/StringExtras.h" to the source file as well.

As Error.h is present in a large number of translation units this means we are unnecessarily bringing in the
contents of StringExtras.h - itself a large file with lots of includes - and slowing down compilation.

Includes have been added to source/header files that are needed but were being transitively included. The
majority needed StringExtras.h but a few only required a smaller header: APInt.h, APSInt.h, or
SmallString.h.

This reduces the total number of preprocessing tokens across the LLVM source files in lib from (roughly)
1,920,413,050 to 1,903,629,230 - a reduction of ~0.87%. This should result in a small improvement in compilation
time.

Diff Detail

Event Timeline

IncludeGuardian requested review of this revision.Jun 18 2023, 10:56 AM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript

This was found with IncludeGuardian 0.0.8.

The line that suggested this can be found in the output here.

The count of preprocessing tokens before and after this change can be found in before.yaml and after.yaml. Note that both these were run on top of https://reviews.llvm.org/D150997, which is yet to make it into the main branch.

Is SmallVector.h still required in Error.h?

Fix compile issues

Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

@barannikov88 no it's not. I was going to commit separately to keep the change small, but it turns out that if I move this to the source file there are no additional changes needed. SmallVector.h has now been moved to Error.cpp as well.

@barannikov88 no it's not. I was going to commit separately to keep the change small, but it turns out that if I move this to the source file there are no additional changes needed. SmallVector.h has now been moved to Error.cpp as well.

I guess it is transitively included from some other header, probably Twine.h.
LGTM with build fixed. I'll leave it for someone else to accept so that it does not disappear from review queue.

Fix missing include

Add back all includes (bad arc change)

Fix missing includes for Windows-only files

MaskRay accepted this revision.Jun 23 2023, 12:51 PM

Seems that the main change is in llvm/include/llvm/Support/Error.h. I think there is a small but not negligible chance that this patch may cause a breakage for some build configurations.
I suggest that you land the #include "llvm/ADT/StringExtras.h" addition part first, then land a much smaller change that modifes llvm/include/llvm/Support/Error.h. So that even if someone detects a failure and decides to revert it, the churn only goes to llvm/include/llvm/Support/Error.h

This revision is now accepted and ready to land.Jun 23 2023, 12:51 PM

@MaskRay good idea, I have split into 2 commits and pushed to the main

Glad that you landed the first change separately:) The Error.cpp change was reverted by b3c8554f28a95063e406924c25336e0da7efb4fd which was more focused.

I've used something like for i in llvm clang clang-tools flang lld lldb bolt mlir; do LIT_OPTS='--filter=xxx --allow-empty-runs' ninja -C /tmp/Rel check-$i to ensure I have built everything, beside a few projects I run check-$i.
It's always good to make the last-minute testing as llvm-project has 100+ commits every day.