Add bit-precise overloads for builtin operators #84755
Merged
Conversation
This file contains 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 previously were not adding them to the candidate set and so use of a bit-precise integer as a class member could lead to ambiguous overload sets. Fixes llvm#82998
AaronBallman
added
c++
clang:frontend
|
@llvm/pr-subscribers-clang Author: Aaron Ballman (AaronBallman) ChangesWe previously were not adding them to the candidate set and so use of a bit-precise integer as a class member could lead to ambiguous overload sets. Fixes #82998 Full diff: https://github.com/llvm/llvm-project/pull/84755.diff 3 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bce27dc8c4a996..5b3a5b6c9bf23e 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -262,6 +262,9 @@ Bug Fixes in This Version
operator.
Fixes (#GH83267).
+- Clang now correctly generates overloads for bit-precise integer types for
+ builtin operators in C++. Fixes #GH82998.
+
Bug Fixes to Compiler Builtins
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index b0c693f078efe2..675f6f9ebcda5f 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -8516,6 +8516,9 @@ class BuiltinCandidateTypeSet {
/// candidates.
TypeSet MatrixTypes;
+ /// The set of _BitInt types that will be used in the built-in candidates.
+ TypeSet BitIntTypes;
+
/// A flag indicating non-record types are viable candidates
bool HasNonRecordTypes;
@@ -8564,6 +8567,7 @@ class BuiltinCandidateTypeSet {
}
llvm::iterator_range<iterator> vector_types() { return VectorTypes; }
llvm::iterator_range<iterator> matrix_types() { return MatrixTypes; }
+ llvm::iterator_range<iterator> bitint_types() { return BitIntTypes; }
bool containsMatrixType(QualType Ty) const { return MatrixTypes.count(Ty); }
bool hasNonRecordTypes() { return HasNonRecordTypes; }
@@ -8735,6 +8739,9 @@ BuiltinCandidateTypeSet::AddTypesConvertedFrom(QualType Ty,
} else if (Ty->isEnumeralType()) {
HasArithmeticOrEnumeralTypes = true;
EnumerationTypes.insert(Ty);
+ } else if (Ty->isBitIntType()) {
+ HasArithmeticOrEnumeralTypes = true;
+ BitIntTypes.insert(Ty);
} else if (Ty->isVectorType()) {
// We treat vector types as arithmetic types in many contexts as an
// extension.
@@ -8955,6 +8962,22 @@ class BuiltinOperatorOverloadBuilder {
(S.Context.getAuxTargetInfo() &&
S.Context.getAuxTargetInfo()->hasInt128Type()))
ArithmeticTypes.push_back(S.Context.UnsignedInt128Ty);
+
+ /// _BitInt overloads are a bit special. We don't want to add candidates
+ /// for the entire family of _BitInt types, so instead we only add
+ /// candidates for the unique, unqualified _BitInt types present in the
+ /// candidate type set. The candidate set already handled ensuring the type
+ /// is unqualified and canonical, but because we're adding from N different
+ /// sets, we need to do some extra work to unique things. Copy the
+ /// candidates into a unique set, then copy from that set into the list of
+ /// arithmetic types.
+ llvm::SmallSetVector<CanQualType, 2> BitIntCandidates;
+ llvm::for_each(CandidateTypes, [&BitIntCandidates](
+ BuiltinCandidateTypeSet &Candidate) {
+ for (QualType BitTy : Candidate.bitint_types())
+ BitIntCandidates.insert(CanQualType::CreateUnsafe(BitTy));
+ });
+ llvm::copy(BitIntCandidates, std::back_inserter(ArithmeticTypes));
LastPromotedIntegralType = ArithmeticTypes.size();
LastPromotedArithmeticType = ArithmeticTypes.size();
// End of promoted types.
diff --git a/clang/test/SemaCXX/overload-bitint.cpp b/clang/test/SemaCXX/overload-bitint.cpp
new file mode 100644
index 00000000000000..b834a3b01fede6
--- /dev/null
+++ b/clang/test/SemaCXX/overload-bitint.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -std=c++20 %s -verify
+// expected-no-diagnostics
+
+#include "Inputs/std-compare.h"
+
+struct S {
+ _BitInt(12) a;
+
+ constexpr operator _BitInt(12)() const { return a; }
+};
+
+// None of these used to compile because we weren't adding _BitInt types to the
+// overload set for builtin operators. See GH82998.
+static_assert(S{10} < 11);
+static_assert(S{10} <= 11);
+static_assert(S{12} > 11);
+static_assert(S{12} >= 11);
+static_assert(S{10} == 10);
+static_assert((S{10} <=> 10) == 0);
+static_assert(S{10} != 11);
+static_assert(S{10} + 0 == 10);
+static_assert(S{10} - 0 == 10);
+static_assert(S{10} * 1 == 10);
+static_assert(S{10} / 1 == 10);
+static_assert(S{10} % 1 == 0);
+static_assert(S{10} << 0 == 10);
+static_assert(S{10} >> 0 == 10);
+static_assert((S{10} | 0) == 10);
+static_assert((S{10} & 10) == 10);
+static_assert((S{10} ^ 0) == 10);
+static_assert(-S{10} == -10);
+static_assert(+S{10} == +10);
+static_assert(~S{10} == ~10);
+
+struct A {
+ _BitInt(12) a;
+
+ bool operator==(const A&) const = default;
+ bool operator!=(const A&) const = default;
+ std::strong_ordering operator<=>(const A&) const = default;
+};
+
|
cor3ntin
reviewed
Mar 11, 2024
clang/lib/Sema/SemaOverload.cpp
Outdated
| for (QualType BitTy : Candidate.bitint_types()) | ||
| BitIntCandidates.insert(CanQualType::CreateUnsafe(BitTy)); | ||
| }); | ||
| llvm::copy(BitIntCandidates, std::back_inserter(ArithmeticTypes)); |
There was a problem hiding this comment.
Suggested change
| llvm::copy(BitIntCandidates, std::back_inserter(ArithmeticTypes)); | |
| llvm::move(BitIntCandidates, std::back_inserter(ArithmeticTypes)); |
clang/lib/Sema/SemaOverload.cpp
Outdated
| /// _BitInt overloads are a bit special. We don't want to add candidates | ||
| /// for the entire family of _BitInt types, so instead we only add | ||
| /// candidates for the unique, unqualified _BitInt types present in the | ||
| /// candidate type set. The candidate set already handled ensuring the type |
There was a problem hiding this comment.
This comment is a bit weird imo. "We don't want to add candidates for the entire family of _BitInt types" seems a bit obvious (and if we wanted to do that, wouldn't we need a lot more special handling any way?
There was a problem hiding this comment.
Sure, I can cut the comment down/remove it.
erichkeane
reviewed
Mar 11, 2024
clang/lib/Sema/SemaOverload.cpp
Outdated
| for (QualType BitTy : Candidate.bitint_types()) | ||
| BitIntCandidates.insert(CanQualType::CreateUnsafe(BitTy)); | ||
| }); | ||
| llvm::copy(BitIntCandidates, std::back_inserter(ArithmeticTypes)); |
There was a problem hiding this comment.
I'm concerned about the assert on 9001. It seems to me that this changes us from being a reasonably 'fixed' list to having a perhaps-unlimited list.
I think that the intent of that assert is nice (ensuring we can small-vector-opt this list), but this section here makes that no longer guaranteed, right?
The ArithmeticTypesCap is 24, and we do a push_back of 23 other types (by my count!). So adding TWO bit-int types + having all the rest of the types enabled causes that assert to fire.
There was a problem hiding this comment.
Good catch! I'll correct this.
We're subtracting out the bit-precise types when determining whether we're under the cap because we don't know for sure how many bit-precise types will wind up in the overload set. This assertion exists to let us know when we've hit a point where we need to allocate rather than use the small vector inline storage, so to avoid needing those allocations, I've bumped the cap up by two elements on the assumption that builtin overload set will often not have more than two candidate types.
erichkeane
approved these changes
Mar 11, 2024
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.
We previously were not adding them to the candidate set and so use of a bit-precise integer as a class member could lead to ambiguous overload sets.
Fixes #82998