[clang] Rework hasBooleanRepresentation.
#136038
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen Author: Michele Scandale (michele-scandale) ChangesThis is a follow-up of 13aac46. This commit adjusts the implementation of Full diff: https://github.com/llvm/llvm-project/pull/136038.diff 5 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 3faf63e395a08..07ada202075a2 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -5238,6 +5238,14 @@ inline bool IsEnumDeclScoped(EnumDecl *ED) {
return ED->isScoped();
}
+/// Return the integer type corresponding to the given decl.
+///
+/// We use this function to break a cycle between the inline definitions in
+/// Type.h and Decl.h.
+inline QualType GetEnumDeclIntegerType(EnumDecl *ED) {
+ return ED->getIntegerType();
+}
+
/// OpenMP variants are mangled early based on their OpenMP context selector.
/// The new name looks likes this:
/// <name> + OpenMPVariantManglingSeparatorStr + <mangled OpenMP context>
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 5bf036e3347eb..ce6904abcc71a 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -2774,7 +2774,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
bool hasFloatingRepresentation() const;
/// Determine whether this type has a boolean representation
- /// of some sort.
+ /// of some sort, e.g., it is a boolean type or a vector thereof.
bool hasBooleanRepresentation() const;
// Type Checking Functions: Check to see if this type is structurally the
@@ -8531,6 +8531,7 @@ inline bool Type::isNullPtrType() const {
bool IsEnumDeclComplete(EnumDecl *);
bool IsEnumDeclScoped(EnumDecl *);
+QualType GetEnumDeclIntegerType(EnumDecl *);
inline bool Type::isIntegerType() const {
if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))
@@ -8623,6 +8624,13 @@ inline bool Type::isIntegralOrEnumerationType() const {
inline bool Type::isBooleanType() const {
if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))
return BT->getKind() == BuiltinType::Bool;
+ if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) {
+ // Incomplete enum types are not treated as integer types.
+ // FIXME: In C++, enum types are never integer types.
+ return IsEnumDeclComplete(ET->getDecl()) &&
+ !IsEnumDeclScoped(ET->getDecl()) &&
+ GetEnumDeclIntegerType(ET->getDecl())->isBooleanType();
+ }
return false;
}
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 53620003c9655..b456f43d39224 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2336,16 +2336,9 @@ bool Type::isArithmeticType() const {
}
bool Type::hasBooleanRepresentation() const {
- if (isBooleanType())
- return true;
-
- if (const EnumType *ET = getAs<EnumType>())
- return ET->getDecl()->getIntegerType()->isBooleanType();
-
- if (const AtomicType *AT = getAs<AtomicType>())
- return AT->getValueType()->hasBooleanRepresentation();
-
- return false;
+ if (const auto *VT = dyn_cast<VectorType>(CanonicalType))
+ return VT->getElementType()->isBooleanType();
+ return isBooleanType();
}
Type::ScalarTypeKind Type::getScalarTypeKind() const {
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index abb88477062fc..786a56eed7ed5 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1920,7 +1920,7 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty,
llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) {
llvm::APInt Min, End;
if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums,
- Ty->hasBooleanRepresentation()))
+ Ty->hasBooleanRepresentation() && !Ty->isVectorType()))
return nullptr;
llvm::MDBuilder MDHelper(getLLVMContext());
@@ -1948,7 +1948,7 @@ bool CodeGenFunction::EmitScalarRangeCheck(llvm::Value *Value, QualType Ty,
if (!HasBoolCheck && !HasEnumCheck)
return false;
- bool IsBool = Ty->hasBooleanRepresentation() ||
+ bool IsBool = (Ty->hasBooleanRepresentation() && !Ty->isVectorType()) ||
NSAPI(CGM.getContext()).isObjCBOOLType(Ty);
bool NeedsBoolCheck = HasBoolCheck && IsBool;
bool NeedsEnumCheck = HasEnumCheck && Ty->getAs<EnumType>();
@@ -2068,11 +2068,8 @@ llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address Addr, bool Volatile,
/// by ConvertType) to its load/store type (as returned by
/// convertTypeForLoadStore).
llvm::Value *CodeGenFunction::EmitToMemory(llvm::Value *Value, QualType Ty) {
- if (Ty->hasBooleanRepresentation() || Ty->isBitIntType()) {
- llvm::Type *StoreTy = convertTypeForLoadStore(Ty, Value->getType());
- bool Signed = Ty->isSignedIntegerOrEnumerationType();
- return Builder.CreateIntCast(Value, StoreTy, Signed, "storedv");
- }
+ if (auto *AtomicTy = Ty->getAs<AtomicType>())
+ Ty = AtomicTy->getValueType();
if (Ty->isExtVectorBoolType()) {
llvm::Type *StoreTy = convertTypeForLoadStore(Ty, Value->getType());
@@ -2088,6 +2085,12 @@ llvm::Value *CodeGenFunction::EmitToMemory(llvm::Value *Value, QualType Ty) {
Value = Builder.CreateBitCast(Value, StoreTy);
}
+ if (Ty->hasBooleanRepresentation() || Ty->isBitIntType()) {
+ llvm::Type *StoreTy = convertTypeForLoadStore(Ty, Value->getType());
+ bool Signed = Ty->isSignedIntegerOrEnumerationType();
+ return Builder.CreateIntCast(Value, StoreTy, Signed, "storedv");
+ }
+
return Value;
}
@@ -2095,6 +2098,9 @@ llvm::Value *CodeGenFunction::EmitToMemory(llvm::Value *Value, QualType Ty) {
/// by convertTypeForLoadStore) to its primary IR type (as returned
/// by ConvertType).
llvm::Value *CodeGenFunction::EmitFromMemory(llvm::Value *Value, QualType Ty) {
+ if (auto *AtomicTy = Ty->getAs<AtomicType>())
+ Ty = AtomicTy->getValueType();
+
if (Ty->isPackedVectorBoolType(getContext())) {
const auto *RawIntTy = Value->getType();
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index eba7267904fb2..427905a4ddafe 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9955,7 +9955,7 @@ QualType Sema::BuiltinChangeSignedness(QualType BaseType, UTTKind UKind,
SourceLocation Loc) {
bool IsMakeSigned = UKind == UnaryTransformType::MakeSigned;
if ((!BaseType->isIntegerType() && !BaseType->isEnumeralType()) ||
- BaseType->isBooleanType() ||
+ (BaseType->isBooleanType() && !BaseType->isEnumeralType()) ||
(BaseType->isBitIntType() &&
BaseType->getAs<BitIntType>()->getNumBits() < 2)) {
Diag(Loc, diag::err_make_signed_integral_only)
|
|
@llvm/pr-subscribers-clang Author: Michele Scandale (michele-scandale) ChangesThis is a follow-up of 13aac46. This commit adjusts the implementation of Full diff: https://github.com/llvm/llvm-project/pull/136038.diff 5 Files Affected:
diff --git a/clang/include/clang/AST/Decl.h b/clang/include/clang/AST/Decl.h
index 3faf63e395a08..07ada202075a2 100644
--- a/clang/include/clang/AST/Decl.h
+++ b/clang/include/clang/AST/Decl.h
@@ -5238,6 +5238,14 @@ inline bool IsEnumDeclScoped(EnumDecl *ED) {
return ED->isScoped();
}
+/// Return the integer type corresponding to the given decl.
+///
+/// We use this function to break a cycle between the inline definitions in
+/// Type.h and Decl.h.
+inline QualType GetEnumDeclIntegerType(EnumDecl *ED) {
+ return ED->getIntegerType();
+}
+
/// OpenMP variants are mangled early based on their OpenMP context selector.
/// The new name looks likes this:
/// <name> + OpenMPVariantManglingSeparatorStr + <mangled OpenMP context>
diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h
index 5bf036e3347eb..ce6904abcc71a 100644
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -2774,7 +2774,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
bool hasFloatingRepresentation() const;
/// Determine whether this type has a boolean representation
- /// of some sort.
+ /// of some sort, e.g., it is a boolean type or a vector thereof.
bool hasBooleanRepresentation() const;
// Type Checking Functions: Check to see if this type is structurally the
@@ -8531,6 +8531,7 @@ inline bool Type::isNullPtrType() const {
bool IsEnumDeclComplete(EnumDecl *);
bool IsEnumDeclScoped(EnumDecl *);
+QualType GetEnumDeclIntegerType(EnumDecl *);
inline bool Type::isIntegerType() const {
if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))
@@ -8623,6 +8624,13 @@ inline bool Type::isIntegralOrEnumerationType() const {
inline bool Type::isBooleanType() const {
if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType))
return BT->getKind() == BuiltinType::Bool;
+ if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) {
+ // Incomplete enum types are not treated as integer types.
+ // FIXME: In C++, enum types are never integer types.
+ return IsEnumDeclComplete(ET->getDecl()) &&
+ !IsEnumDeclScoped(ET->getDecl()) &&
+ GetEnumDeclIntegerType(ET->getDecl())->isBooleanType();
+ }
return false;
}
diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
index 53620003c9655..b456f43d39224 100644
--- a/clang/lib/AST/Type.cpp
+++ b/clang/lib/AST/Type.cpp
@@ -2336,16 +2336,9 @@ bool Type::isArithmeticType() const {
}
bool Type::hasBooleanRepresentation() const {
- if (isBooleanType())
- return true;
-
- if (const EnumType *ET = getAs<EnumType>())
- return ET->getDecl()->getIntegerType()->isBooleanType();
-
- if (const AtomicType *AT = getAs<AtomicType>())
- return AT->getValueType()->hasBooleanRepresentation();
-
- return false;
+ if (const auto *VT = dyn_cast<VectorType>(CanonicalType))
+ return VT->getElementType()->isBooleanType();
+ return isBooleanType();
}
Type::ScalarTypeKind Type::getScalarTypeKind() const {
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index abb88477062fc..786a56eed7ed5 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -1920,7 +1920,7 @@ static bool getRangeForType(CodeGenFunction &CGF, QualType Ty,
llvm::MDNode *CodeGenFunction::getRangeForLoadFromType(QualType Ty) {
llvm::APInt Min, End;
if (!getRangeForType(*this, Ty, Min, End, CGM.getCodeGenOpts().StrictEnums,
- Ty->hasBooleanRepresentation()))
+ Ty->hasBooleanRepresentation() && !Ty->isVectorType()))
return nullptr;
llvm::MDBuilder MDHelper(getLLVMContext());
@@ -1948,7 +1948,7 @@ bool CodeGenFunction::EmitScalarRangeCheck(llvm::Value *Value, QualType Ty,
if (!HasBoolCheck && !HasEnumCheck)
return false;
- bool IsBool = Ty->hasBooleanRepresentation() ||
+ bool IsBool = (Ty->hasBooleanRepresentation() && !Ty->isVectorType()) ||
NSAPI(CGM.getContext()).isObjCBOOLType(Ty);
bool NeedsBoolCheck = HasBoolCheck && IsBool;
bool NeedsEnumCheck = HasEnumCheck && Ty->getAs<EnumType>();
@@ -2068,11 +2068,8 @@ llvm::Value *CodeGenFunction::EmitLoadOfScalar(Address Addr, bool Volatile,
/// by ConvertType) to its load/store type (as returned by
/// convertTypeForLoadStore).
llvm::Value *CodeGenFunction::EmitToMemory(llvm::Value *Value, QualType Ty) {
- if (Ty->hasBooleanRepresentation() || Ty->isBitIntType()) {
- llvm::Type *StoreTy = convertTypeForLoadStore(Ty, Value->getType());
- bool Signed = Ty->isSignedIntegerOrEnumerationType();
- return Builder.CreateIntCast(Value, StoreTy, Signed, "storedv");
- }
+ if (auto *AtomicTy = Ty->getAs<AtomicType>())
+ Ty = AtomicTy->getValueType();
if (Ty->isExtVectorBoolType()) {
llvm::Type *StoreTy = convertTypeForLoadStore(Ty, Value->getType());
@@ -2088,6 +2085,12 @@ llvm::Value *CodeGenFunction::EmitToMemory(llvm::Value *Value, QualType Ty) {
Value = Builder.CreateBitCast(Value, StoreTy);
}
+ if (Ty->hasBooleanRepresentation() || Ty->isBitIntType()) {
+ llvm::Type *StoreTy = convertTypeForLoadStore(Ty, Value->getType());
+ bool Signed = Ty->isSignedIntegerOrEnumerationType();
+ return Builder.CreateIntCast(Value, StoreTy, Signed, "storedv");
+ }
+
return Value;
}
@@ -2095,6 +2098,9 @@ llvm::Value *CodeGenFunction::EmitToMemory(llvm::Value *Value, QualType Ty) {
/// by convertTypeForLoadStore) to its primary IR type (as returned
/// by ConvertType).
llvm::Value *CodeGenFunction::EmitFromMemory(llvm::Value *Value, QualType Ty) {
+ if (auto *AtomicTy = Ty->getAs<AtomicType>())
+ Ty = AtomicTy->getValueType();
+
if (Ty->isPackedVectorBoolType(getContext())) {
const auto *RawIntTy = Value->getType();
diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index eba7267904fb2..427905a4ddafe 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -9955,7 +9955,7 @@ QualType Sema::BuiltinChangeSignedness(QualType BaseType, UTTKind UKind,
SourceLocation Loc) {
bool IsMakeSigned = UKind == UnaryTransformType::MakeSigned;
if ((!BaseType->isIntegerType() && !BaseType->isEnumeralType()) ||
- BaseType->isBooleanType() ||
+ (BaseType->isBooleanType() && !BaseType->isEnumeralType()) ||
(BaseType->isBitIntType() &&
BaseType->getAs<BitIntType>()->getNumBits() < 2)) {
Diag(Loc, diag::err_make_signed_integral_only)
|
clang/include/clang/AST/Type.h
Outdated
| inline bool Type::isBooleanType() const { | ||
| if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) | ||
| return BT->getKind() == BuiltinType::Bool; | ||
| if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) { |
There was a problem hiding this comment.
This is to match isIntegerType that is used by hasIntegerRepresentation.
I'm not entirely sure it makes sense to handle enums here (given also the FIXME comment).
I'll try to see what breaks by removing this.
There was a problem hiding this comment.
From a local run of check-clang there are no tests failing with this part of the code. However I can see on small examples how EmitFromMemory and EmitToMemory are not doing anything to converts between i1 and i8 -- which seems wrong.
There was a problem hiding this comment.
I'm not entirely sure it makes sense to handle enums here
I don't believe it makes sense to report true for an enum type. An enumeration type is never a boolean type in C or in C++. An enumeration can have a boolean underlying type, but it's still an enumeration type rather than a boolean type.
andykaylor
left a comment
andykaylor
left a comment
There was a problem hiding this comment.
This looks reasonable to me.
| Value = Builder.CreateBitCast(Value, StoreTy); | ||
| } | ||
|
|
||
| if (Ty->hasBooleanRepresentation() || Ty->isBitIntType()) { |
There was a problem hiding this comment.
Is isExtVectorBoolType() guaranteed to filter out all the cases where a boolean vector type might get us here? It doesn't seem like the previous code had any sort of handling for vectors of bool other than the isExtVectorBoolType handling, but this seems like a potential change in behavior.
There was a problem hiding this comment.
I'll double check. I know there have been changes recently to support a different ABI in the HLSL case.
There was a problem hiding this comment.
Only ExtVectorType of booleans are supported, the regular VectorType disallow boolean element types (see Sema::BuildVectorType and Sema::BuildExtVectorType).
In the if (Ty->isExtVectorBoolType()) above both kind of vector bool ABIs are handled, so there is no issue there.
| /// Converts a scalar value from its load/store type (as returned | ||
| /// by convertTypeForLoadStore) to its primary IR type (as returned | ||
| /// by ConvertType). | ||
| llvm::Value *CodeGenFunction::EmitFromMemory(llvm::Value *Value, QualType Ty) { |
There was a problem hiding this comment.
In here it is fine to handle the vector of bool types whenTy->hasBooleanRepresentation() as only the non packed ABI will reach that code, and it is fine to generate a Trunc (just like in EmitToMemory a ZExt is generated)
clang/include/clang/AST/Type.h
Outdated
| inline bool Type::isBooleanType() const { | ||
| if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) | ||
| return BT->getKind() == BuiltinType::Bool; | ||
| if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) { |
There was a problem hiding this comment.
| if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) { | |
| if (const auto *ET = dyn_cast<EnumType>(CanonicalType)) { |
clang/include/clang/AST/Decl.h
Outdated
| /// | ||
| /// We use this function to break a cycle between the inline definitions in | ||
| /// Type.h and Decl.h. | ||
| inline QualType GetEnumDeclIntegerType(EnumDecl *ED) { |
There was a problem hiding this comment.
Can we make the parameter const?
There was a problem hiding this comment.
We've generally been naming new functions with lower-case letters.
clang/include/clang/AST/Type.h
Outdated
| inline bool Type::isBooleanType() const { | ||
| if (const auto *BT = dyn_cast<BuiltinType>(CanonicalType)) | ||
| return BT->getKind() == BuiltinType::Bool; | ||
| if (const EnumType *ET = dyn_cast<EnumType>(CanonicalType)) { |
There was a problem hiding this comment.
I'm not entirely sure it makes sense to handle enums here
I don't believe it makes sense to report true for an enum type. An enumeration type is never a boolean type in C or in C++. An enumeration can have a boolean underlying type, but it's still an enumeration type rather than a boolean type.
clang/include/clang/AST/Type.h
Outdated
| // Incomplete enum types are not treated as integer types. | ||
| // FIXME: In C++, enum types are never integer types. | ||
| return IsEnumDeclComplete(ET->getDecl()) && | ||
| !IsEnumDeclScoped(ET->getDecl()) && |
There was a problem hiding this comment.
Why does the the enum being scoped matter?
There was a problem hiding this comment.
This is just the same code that exist in isIntegerType. I am puzzled by this code as well, but I suppose there is a good reason for it.
There was a problem hiding this comment.
C specifies that "enumerated types" are integer types, and C++ does not, and isIntegerType is trying to honor that as best it can. It's one of those situations where we need to ask what callers actually want, because in most cases it's probably not this language-specific formal property. If hasIntegerRepresentation is really a question about the representation of the type, it should be ignoring scoped-ness of enums. But it seems to be used in Sema in ways that should definitely exclude scoped enums, like the semantic analysis of __builtin_shuffle_vector and vector math.
isBooleanType should probably continue to check only for bool. It seems reasonable for hasBooleanRepresentation to look through enums, though, and it should probably ignore the scoped-ness of the enum.
There was a problem hiding this comment.
C specifies that "enumerated types" are integer types, and C++ does not, and
isIntegerTypeis trying to honor that as best it can. It's one of those situations where we need to ask what callers actually want, because in most cases it's probably not this language-specific formal property. IfhasIntegerRepresentationis really a question about the representation of the type, it should be ignoring scoped-ness of enums. But it seems to be used in Sema in ways that should definitely exclude scoped enums, like the semantic analysis of__builtin_shuffle_vectorand vector math.
The check on scoped enum comes from:
commit 21673c4e7ec08457b53798b9879b7cc9a5909eb8
Author: Douglas Gregor <[email protected]>
Date: Thu May 5 16:13:52 2011 +0000
Scoped enumerations should not be treated as integer types (in the C
sense). Fixes <rdar://problem/9366066> by eliminating an inconsistency
between C++ overloading (which handled scoped enumerations correctly)
and C binary operator type-checking (which didn't).
llvm-svn: 130924
isBooleanTypeshould probably continue to check only forbool.
Ok.
It seems reasonable for
hasBooleanRepresentationto look through enums, though, and it should probably ignore the scoped-ness of the enum.
I can do that, but I'm not super happy about the inconsistency.
There was a problem hiding this comment.
Okay. My understanding is that supporting CodeGen and its dual-representation handling of boolean types is the immediate purpose of this method. "Representation" is a good name for such a method because this really is about lower-level representation.
At a high level, scoped enums with an underlying type of bool either need to use i8 as both their memory and their scalar lowered type or they need to be doing toMemory/fromMemory operations when moving to/from memory. I think the latter is probably the better situation overall. So I think we really want this method to ignored scoped-ness of enums.
The immediate problem this creates is that we have a bunch of existing methods that are arguably misnamed: they're named something involving "representation", but they're not really being used to ask about low-level representation. Instead, they're vector-inclusive variants of other predicates. I agree this is a bad place to have ended up, because people will naturally expect all the similarly-named methods to behave consistently.
Since the existing methods are arguably misnamed, I wonder if we could reasonably just change them. Since the difference is (AFAIK) purely to make them vector-inclusive, perhaps we should just remove them and have a getNonVectorType() method that clients are expected to call before using the existing predicates.
There was a problem hiding this comment.
Since the existing methods are arguably misnamed, I wonder if we could reasonably just change them. Since the difference is (AFAIK) purely to make them vector-inclusive, perhaps we should just remove them and have a getNonVectorType() method that clients are expected to call before using the existing predicates.
Just to make sure I'm not misunderstanding things: are you suggesting to just drop the "vector of" portion of the has*Representation and move that onto clients of these APIs, right?
I'm not sure if that can work in general. E.g.
bool Type::hasIntegerRepresentation() const {
if (const auto *VT = dyn_cast<VectorType>(CanonicalType))
return VT->getElementType()->isIntegerType();
if (CanonicalType->isSveVLSBuiltinType()) {
const auto *VT = cast<BuiltinType>(CanonicalType);
return VT->getKind() == BuiltinType::SveBool ||
(VT->getKind() >= BuiltinType::SveInt8 &&
VT->getKind() <= BuiltinType::SveUint64);
}
if (CanonicalType->isRVVVLSBuiltinType()) {
const auto *VT = cast<BuiltinType>(CanonicalType);
return (VT->getKind() >= BuiltinType::RvvInt8mf8 &&
VT->getKind() <= BuiltinType::RvvUint64m8);
}
return isIntegerType();
}
This is more than just types where isIntegerType is true, or vector of such types.
There was a problem hiding this comment.
I see. I'm not sure I understand why someone added a bunch of new vector types as individual builtin types (from the names, they sure seem to have element types and otherwise fit the general vector model!), but if that's what we have to deal with, then I suppose I'm just arguing that the methods should be renamed to not say "representation".
clang/include/clang/AST/Decl.h
Outdated
| /// | ||
| /// We use this function to break a cycle between the inline definitions in | ||
| /// Type.h and Decl.h. | ||
| inline QualType GetEnumDeclIntegerType(EnumDecl *ED) { |
There was a problem hiding this comment.
We've generally been naming new functions with lower-case letters.
clang/include/clang/AST/Type.h
Outdated
|
|
||
| /// Determine whether this type has a boolean representation | ||
| /// of some sort. | ||
| /// of some sort, e.g., it is a boolean type or a vector thereof. |
There was a problem hiding this comment.
| /// of some sort, e.g., it is a boolean type or a vector thereof. | |
| /// of some sort, i.e., it is a boolean type or a vector thereof. |
clang/include/clang/AST/Type.h
Outdated
| // Incomplete enum types are not treated as integer types. | ||
| // FIXME: In C++, enum types are never integer types. | ||
| return IsEnumDeclComplete(ET->getDecl()) && | ||
| !IsEnumDeclScoped(ET->getDecl()) && |
There was a problem hiding this comment.
C specifies that "enumerated types" are integer types, and C++ does not, and isIntegerType is trying to honor that as best it can. It's one of those situations where we need to ask what callers actually want, because in most cases it's probably not this language-specific formal property. If hasIntegerRepresentation is really a question about the representation of the type, it should be ignoring scoped-ness of enums. But it seems to be used in Sema in ways that should definitely exclude scoped enums, like the semantic analysis of __builtin_shuffle_vector and vector math.
isBooleanType should probably continue to check only for bool. It seems reasonable for hasBooleanRepresentation to look through enums, though, and it should probably ignore the scoped-ness of the enum.
michele-scandale
force-pushed
the
hasBooleanRepresentation-rework
branch
from
31777c1 to
17e7377
Compare
|
Putting aside for now the naming issue (#136038 (comment)), is there anything left for the changes proposed in this PR? |
michele-scandale
requested review from
AaronBallman,
andykaylor,
rjmccall and
tbaederr
Based on:
I think this probably should include 1-bit bit-precise integer types in |
This is a follow-up of 13aac46. This commit adjusts the implementation of `hasBooleanRepresentation` to somewhat aligned to `hasIntegerRepresentation`. In particular vector of booleans should be handled in `hasBooleanRepresentation`, while `_Atomic(bool)` should not.
michele-scandale
force-pushed
the
hasBooleanRepresentation-rework
branch
from
17e7377 to
359e31e
Compare
|
@rjmccall Are you ok landing this change, and defer the renaming to a separate change? |
|
That's fine with me, yeah. |
This is a follow-up of 13aac46. This commit adjusts the implementation of `hasBooleanRepresentation` to be somewhat aligned to `hasIntegerRepresentation`. In particular vector of booleans should be handled in `hasBooleanRepresentation`, while `_Atomic(bool)` should not.
6 participants
This is a follow-up of 13aac46. This commit adjusts the implementation of
hasBooleanRepresentationto have a similar behavior as the one ofhasIntegerRepresentation. In particular vector of booleans should be handled inhasBooleanRepresentation, while_Atomic(bool)should not.