-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[Clang] Normalize constraints before checking for satisfaction #161671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Clang] Normalize constraints before checking for satisfaction #161671
Conversation
We substitute into concept ids for diagnostics purposes. However, most of the time this happen in SFINEA context and impact performance noticeably.
We may substitute _Fn::template __f (a dependent template name) into a template-template parameter name, when building a parameter mapping. They can't have the same NNSLoc, so the assertion doesn't hold. Also we missed a case when profiling the concept, as in InjectedClassNameType.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
We're seeing significant (16%) build time increases after this revision. I'll try to come up with some kind of repro, but wanted to give a heads up. |
|
Here's a random translation unit where compile time increased about 10%: plugin_service_impl.ii.gz |
|
@zmodem Thanks, I'll investigate. Note that we expected some performance degradation on very concept heavy codebase without having a good way to quantify it - and it's not clear how much of that we can claw back, but we will certainly try to improve that as much as we can - I certainly agree that 10-15% is a lot There are still places where our concept implementations is non-conforming so in theory we would have to make the performance worse still, but these non-conformances are less observables so we'll try to focus on improving performances before doing anything else to our concepts implementation. |
|
I discussed this with Hans today, and we think one way to evaluate the performance impact would be to bootstrap clang itself with C++20 enabled. I think Chromium, and probably most C++ code in the wild, isn't using C++20 concepts heavily, and is mostly picking them up through libc++. I really would not want to eat a 16% C++ compile time regression on normal C++ code. As a point of comparison, we're using 32-bit source locations to avoid a 4% peak memory usage increase with 0% measurable regression, which is very conservative. It's hard, though, since it's a performance vs conformance (arguably correctness) tradeoff, it's just not a correctness regression, which we usually weight very highly when making tradeoff decisions. :) |
…ion. When establishing constraint satisfaction, we were building expressions even for compound constraint, This is unecessary extra work that acount for ~20% of the performance regression observed here llvm#161671 (comment)
…ion. When establishing constraint satisfaction, we were building expressions even for compound constraint, This is unecessary extra work that acount for ~20% of the performance regression observed here llvm#161671 (comment)
…tion (#164611) When establishing constraint satisfaction, we were building expressions even for compound constraint, This is unnecessary extra work that accounts for ~20% of the performance regression observed here #161671 (comment)
…ng satisfaction (#164611) When establishing constraint satisfaction, we were building expressions even for compound constraint, This is unnecessary extra work that accounts for ~20% of the performance regression observed here llvm/llvm-project#161671 (comment)
|
I bisected a breakage to this commit, and while I'm still unsure whether the code is breaking is valid or not, it's behaving unexpectedly: the breakage is affected by other template instantiations. I suspect something is wrong with this patch. Reduced/filed as #164750. |
@rupprecht #164777 will fix it. Thanks for the heads up! |
|
Here's another example TU. |
I looked into this, building a stage0 clang and libc++ configured with: and then using that to do a debug shared-library build of clang in c++20 mode: The increase from this change is small but measurable: about 5 CPU-minutes, or 1.5 %. I suppose it makes sense that the difference is smaller, since this codebase has no use of concepts at all besides via the STL, as opposed to Chromium which does seem to have a fair amount of concepts use in its base library, Abseil and so on. I'm not sure how to proceed here. A 16% build time increase (as mentioned up-thread) is very significant for us. @AaronBallman do you have any thoughts here? I did see that #61811 mention "We should get performance data for that change and give feedback to WG21 if it looks like this is going to have a significant performance impact." So it sounds like this was somewhat anticipated?
I don't know much about concepts, but is there some way we could keep the old, fast, code path for most cases, and only use the new more expensive logic when that's not sufficient? |
16% is a bigger performance regression than I think we were anticipating.
I think we expected a performance hit, but more like 1-3%, not 10+%; that's significant enough that if we can't solve it, it should be feedback given to WG21 because that feels like too much of a regression to swallow. CC @cor3ntin (it may be a while before he responds, the dev conference is next week and WG21 meetings are the week after). |
|
Note that I already pushed something that reduces the performance cost by a few percents. both me and Younan will keep exploring additional mitigations in the coming weeks. We think there are specific expressions which are unexpectedly costly, we are still trying to isolate exactly the problematic cases. |
|
Thanks both! See you at the dev meeting :) |
…tion (llvm#164611) When establishing constraint satisfaction, we were building expressions even for compound constraint, This is unnecessary extra work that accounts for ~20% of the performance regression observed here llvm#161671 (comment)
…tion (llvm#164611) When establishing constraint satisfaction, we were building expressions even for compound constraint, This is unnecessary extra work that accounts for ~20% of the performance regression observed here llvm#161671 (comment)
The logical or expression should be parenthesized. The issue was brought by llvm#161671 Fixes llvm#164104
…tion (llvm#164611) When establishing constraint satisfaction, we were building expressions even for compound constraint, This is unnecessary extra work that accounts for ~20% of the performance regression observed here llvm#161671 (comment)
|
Hi @cor3ntin , we've started seeing Clang crashes after this commit. They only reproduce with our Clang header modules setup on a pretty large input. Reduction has been crawling slowly, so I don't expect to get a nice little test case soon (or ever). The crashes are sporadic, but reproduce more or less consistently when clang is built with msan: It looks like serialization or deserialization issue. |
I see. We'll try to dig a bit deeper ourselves, maybe we'll get somewhere before @cor3ntin is back. And the test case reduction is running in the background (it's 30% through now, hopefully, there will be more progress by the end of the week). |
In the standard, constraint satisfaction checking is done on the normalized form of a constraint.
Clang instead substitutes on the non-normalized form, which causes us to report substitution failures in template arguments or concept ids, which is non-conforming but unavoidable without a parameter mapping
This patch normalizes before satisfaction checking. However, we preserve concept-id nodes in the normalized form, solely for diagnostics purposes.
This addresses #61811 and related concepts conformance bugs, ideally to make the remaining implementation of concept template parameters easier
Fixes #135190
Fixes #61811
Co-authored-by: Younan Zhang [email protected]