This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Improve compile times when forming a DeclRef outside of a capturing scope.
ClosedPublic

Authored by cor3ntin on May 6 2023, 6:28 AM.

Details

Summary

The logic of whether an entity needs to be captured has become
quite complex and the recent changes in https://reviews.llvm.org/D124351
ad a mesurable negative impact on compile times.

However, in the absence of capturing scopes (lambda, block, region)
we usually can avoid running most of that logic
(except that we do need to diagnostic when a nested function
refers to a local variable in the scope of the outer function.).

This patch track whether there is currently an active capturing
scope and exit tryCaptureVariable early when there isn't.

Diff Detail

Event Timeline

cor3ntin created this revision.May 6 2023, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2023, 6:28 AM
cor3ntin requested review of this revision.May 6 2023, 6:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2023, 6:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Do you have some performance measurement numbers for how much benefit we get from the changes?

clang/lib/Sema/SemaExpr.cpp
19226
19230

Do you have some performance measurement numbers for how much benefit we get from the changes?

Not really.
The PR that changed the scope of trailing return type had a 0.35% regression, I expect this to be in the same ballpark in the other direction
http://llvm-compile-time-tracker.com/compare.php?from=cd173cbd7cca69c29df42cd4b42e60433435c29b&to=d708a186b6a9b050d09558163dd353d9f738c82d&stat=instructions%3Au

Do you have some performance measurement numbers for how much benefit we get from the changes?

Not really.
The PR that changed the scope of trailing return type had a 0.35% regression, I expect this to be in the same ballpark in the other direction
http://llvm-compile-time-tracker.com/compare.php?from=cd173cbd7cca69c29df42cd4b42e60433435c29b&to=d708a186b6a9b050d09558163dd353d9f738c82d&stat=instructions%3Au

We usually want performance-related changes to come with some hard measurements because of how painfully easy it is to think something will result in a positive performance change that ends up being a wash. Would you mind putting a branch up on the compile time tracker page to validate your expectations? .35% better performance would be nice to see!

Do you have some performance measurement numbers for how much benefit we get from the changes?

Not really.
The PR that changed the scope of trailing return type had a 0.35% regression, I expect this to be in the same ballpark in the other direction
http://llvm-compile-time-tracker.com/compare.php?from=cd173cbd7cca69c29df42cd4b42e60433435c29b&to=d708a186b6a9b050d09558163dd353d9f738c82d&stat=instructions%3Au

We usually want performance-related changes to come with some hard measurements because of how painfully easy it is to think something will result in a positive performance change that ends up being a wash. Would you mind putting a branch up on the compile time tracker page to validate your expectations? .35% better performance would be nice to see!

Ask and you shall receive http://llvm-compile-time-tracker.com/?remote=cor3ntin

aaron.ballman accepted this revision.May 8 2023, 9:32 AM

Fantastic, that's some nice green in there. LGTM modulo nits, please add NFC to the patch summary when landing so folks know not to expect test coverage.

This revision is now accepted and ready to land.May 8 2023, 9:32 AM
This revision was landed with ongoing or failed builds.May 8 2023, 9:41 AM
This revision was automatically updated to reflect the committed changes.