This is an archive of the discontinued LLVM Phabricator instance.

Narrow inline namespace filtration for unqualified friend declarations
ClosedPublic

Authored by troyj on Oct 6 2022, 8:18 AM.

Details

Summary

rG04ba1856 introduced a call to FilterLookupForScope that is expensive for very large translation units where it was observed to cause a 6% compile time degradation. As the comment states, the only effect is to "remove declarations found in inline namespaces for friend declarations with unqualified names." This change limits the call to that scenario. The test that was added by rG04ba1856 continues to pass, but the observed degradation is cut in half.

Diff Detail

Event Timeline

troyj created this revision.Oct 6 2022, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 8:18 AM
troyj requested review of this revision.Oct 6 2022, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 8:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
troyj updated this revision to Diff 465759.Oct 6 2022, 8:59 AM

Pulled and regenerated diff. I'm not sure what the problem was and it still looks the same.

bruno accepted this revision.Oct 6 2022, 5:15 PM
bruno added a reviewer: aaron.ballman.
bruno added subscribers: aaron.ballman, bruno.

Hi Troy, thanks for working on this, nice compile time perf savings. Can you update the diff to contain more context? If anything, -U 999999 or similar would do the job.

@aaron.ballman wdyt?

This revision is now accepted and ready to land.Oct 6 2022, 5:15 PM
shafik added a subscriber: shafik.Oct 6 2022, 8:40 PM
shafik added inline comments.
clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
2277–2278

Nitpick /*Scope=*/nullptr and /*ConsiderLinkage=*/true

troyj updated this revision to Diff 466072.Oct 7 2022, 7:32 AM

Added more diff context as requested.

troyj marked an inline comment as done.Oct 7 2022, 7:33 AM
aaron.ballman accepted this revision.Oct 7 2022, 12:20 PM

LGTM! Should we also add a release note or do we think this isn't enough of a compile-time performance improvement to warrant that?

This revision was landed with ongoing or failed builds.Oct 10 2022, 8:35 AM
This revision was automatically updated to reflect the committed changes.

LGTM! Should we also add a release note or do we think this isn't enough of a compile-time performance improvement to warrant that?

Good point @aaron.ballman, given we have only observed in a few corner case large TUs I'd say it's probably not that normally perceivable. Do you agree @troyj ?

I agree. I wouldn't want to mislead people into thinking that they'll see this improvement on everything.

shafik added a comment.Apr 6 2023, 1:50 PM

It looks like this causes a regression, we can see in this bug report: https://github.com/llvm/llvm-project/issues/61851

shafik added a comment.Apr 6 2023, 2:10 PM

A quick test indicated the !FunctionTemplate is the culprit for the noted regression.