[Clang] Normalize constraints before checking for satisfaction #161671
+2,644
−1,201
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
…n/use_normalization_for_satisfaction
|
Re-pushing #141776 while i investigate CI failures |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
…n/use_normalization_for_satisfaction
MixedMatched
pushed a commit
to MixedMatched/llvm-project
that referenced
this pull request
Oct 3, 2025
…161671) 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 llvm#61811 and related concepts conformance bugs, ideally to make the remaining implementation of concept template parameters easier Fixes llvm#135190 Fixes llvm#61811 Co-authored-by: Younan Zhang [[email protected]](mailto:[email protected]) --------- Co-authored-by: Younan Zhang <[email protected]>
|
After this PR we've started to see static-assert failures when building chromium. I've attached a minimized reproducer (from cvise, then manually cut down as much as I could). The below code will fail after this PR, and succeed before it, when Unfortunately I don't totally understand the code myself, but this seems wrong: we certainly don't want to be able to initialize a 4-element thing from a 3-element thing, and the Furthermore, we need both static_asserts at the end of the file. Whichever happens second will fail, and if only one exists it will pass. |
|
we are also seeing downstream failures related to this PR, seems to be with older GCC toolchains such as found on SLES 15 sp6. maybe revert and fix ? /longer_pathname_so_that_rpms_can_support_packaging_the_debug_info_for_all_os_profiles/src/rocm_bandwidth_test/deps/work_bench/src/task_mgmt.cpp:435:5: error: no matching function for call to object of type 'const __copy_fn' |
|
@ronlieb Can you provide us with a self-contained example? |
zyn0217
added a commit
that referenced
this pull request
Oct 5, 2025
This expression is not handled by default in RAV, so our parameter mapping and cache mechanism don't work when it appears in a template argument list. There are a few other expressions, such as PackIndexingExpr and FunctionParmPackExpr, which are also no-ops by default. I don't have a test case for them now, so let's leave those until users complain :/ There was also a bug in updating the parameter mapping, where the AssociatedDecl was not updated accordingly. Also also, this fixes another regression reported in #161671 (comment), where we failed to account for the variable initializer in cache profiling. Relies on #161671 Fixes #161983 Fixes #161987
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Oct 5, 2025
This expression is not handled by default in RAV, so our parameter mapping and cache mechanism don't work when it appears in a template argument list. There are a few other expressions, such as PackIndexingExpr and FunctionParmPackExpr, which are also no-ops by default. I don't have a test case for them now, so let's leave those until users complain :/ There was also a bug in updating the parameter mapping, where the AssociatedDecl was not updated accordingly. Also also, this fixes another regression reported in llvm/llvm-project#161671 (comment), where we failed to account for the variable initializer in cache profiling. Relies on #161671 Fixes llvm/llvm-project#161983 Fixes llvm/llvm-project#161987
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Oct 5, 2025
llvm#161671)" This reverts commit e9972de.
|
@ronlieb can you try again with trunk ? it should be fixed by 92d8313 It was reproducible with #include <string>
#include <ranges>
#include <array>
#include <algorithm>
std::array<char, 10> current_thread_name{};
auto test(const std::string& task_name) -> void
{
// Initialize the task management
std::ranges::fill(current_thread_name, '\0');
std::ranges::copy(task_name | std::views::take(10 - 1), current_thread_name.begin());
} |
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Oct 5, 2025
…161671) 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 llvm#61811 and related concepts conformance bugs, ideally to make the remaining implementation of concept template parameters easier Fixes llvm#135190 Fixes llvm#61811 Co-authored-by: Younan Zhang [[email protected]](mailto:[email protected]) --------- Co-authored-by: Younan Zhang <[email protected]>
aokblast
pushed a commit
to aokblast/llvm-project
that referenced
this pull request
Oct 6, 2025
This expression is not handled by default in RAV, so our parameter mapping and cache mechanism don't work when it appears in a template argument list. There are a few other expressions, such as PackIndexingExpr and FunctionParmPackExpr, which are also no-ops by default. I don't have a test case for them now, so let's leave those until users complain :/ There was also a bug in updating the parameter mapping, where the AssociatedDecl was not updated accordingly. Also also, this fixes another regression reported in llvm#161671 (comment), where we failed to account for the variable initializer in cache profiling. Relies on llvm#161671 Fixes llvm#161983 Fixes llvm#161987
|
Hey, this breaks our internal range-v3: Seems others hit some related issues too and there are some fixes, but I just checked the error are still present for the HEAD, any idea for the fix? |
|
@wlei-llvm We would appreciate it if you can provide us with a self-contained reproducer, otherwise there's nothing we can do. |
Hi @zyn0217 , here is a reproducer: test.cpp: And it can be reproduced by the Cmd: Error: Thanks! |
|
@wlei-llvm thanks for your example. I'm running a reduction with it and we'll look into it soon once we have it reduced. |
|
@wlei-llvm #162272 should fix it :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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]