[clang][Sema] Move computing best enum types to a separate function #120965
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
kuilpd
added
clang
|
As of now LLDB doesn't calculate enum's best promotion type when reading it from DWARF info. This change would allow to reuse the code from Sema without copying it to LLDB, which I'm working on in #115005 |
|
@llvm/pr-subscribers-clang Author: Ilia Kuklin (kuilpd) ChangesMove the code that calculates BestWidth, BestType and BestPromotionType for an enum to a separate function which can be called from outside of Sema. Full diff: https://github.com/llvm/llvm-project/pull/120965.diff 2 Files Affected:
diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 5ee7ea48cc983c..51a1721fb74f01 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -3987,6 +3987,13 @@ class Sema final : public SemaBase {
SourceLocation IdLoc, IdentifierInfo *Id,
const ParsedAttributesView &Attrs,
SourceLocation EqualLoc, Expr *Val);
+
+ bool ComputeBestEnumProperties(ASTContext &Context, EnumDecl *Enum,
+ bool isCpp, bool isPacked,
+ unsigned NumNegativeBits,
+ unsigned NumPositiveBits, unsigned &BestWidth,
+ QualType &BestType,
+ QualType &BestPromotionType);
void ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
Decl *EnumDecl, ArrayRef<Decl *> Elements, Scope *S,
const ParsedAttributesView &Attr);
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 4001c4d263f1d2..79cbfe3116b26b 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -20008,6 +20008,87 @@ bool Sema::IsValueInFlagEnum(const EnumDecl *ED, const llvm::APInt &Val,
return !(FlagMask & Val) || (AllowMask && !(FlagMask & ~Val));
}
+bool Sema::ComputeBestEnumProperties(ASTContext &Context, EnumDecl *Enum,
+ bool is_cpp, bool isPacked,
+ unsigned NumNegativeBits,
+ unsigned NumPositiveBits,
+ unsigned &BestWidth, QualType &BestType,
+ QualType &BestPromotionType) {
+ unsigned IntWidth = Context.getTargetInfo().getIntWidth();
+ unsigned CharWidth = Context.getTargetInfo().getCharWidth();
+ unsigned ShortWidth = Context.getTargetInfo().getShortWidth();
+ bool enum_too_large = false;
+ if (NumNegativeBits) {
+ // If there is a negative value, figure out the smallest integer type (of
+ // int/long/longlong) that fits.
+ // If it's packed, check also if it fits a char or a short.
+ if (isPacked && NumNegativeBits <= CharWidth &&
+ NumPositiveBits < CharWidth) {
+ BestType = Context.SignedCharTy;
+ BestWidth = CharWidth;
+ } else if (isPacked && NumNegativeBits <= ShortWidth &&
+ NumPositiveBits < ShortWidth) {
+ BestType = Context.ShortTy;
+ BestWidth = ShortWidth;
+ } else if (NumNegativeBits <= IntWidth && NumPositiveBits < IntWidth) {
+ BestType = Context.IntTy;
+ BestWidth = IntWidth;
+ } else {
+ BestWidth = Context.getTargetInfo().getLongWidth();
+
+ if (NumNegativeBits <= BestWidth && NumPositiveBits < BestWidth) {
+ BestType = Context.LongTy;
+ } else {
+ BestWidth = Context.getTargetInfo().getLongLongWidth();
+
+ if (NumNegativeBits > BestWidth || NumPositiveBits >= BestWidth)
+ enum_too_large = true;
+ BestType = Context.LongLongTy;
+ }
+ }
+ BestPromotionType = (BestWidth <= IntWidth ? Context.IntTy : BestType);
+ } else {
+ // If there is no negative value, figure out the smallest type that fits
+ // all of the enumerator values.
+ // If it's packed, check also if it fits a char or a short.
+ if (isPacked && NumPositiveBits <= CharWidth) {
+ BestType = Context.UnsignedCharTy;
+ BestPromotionType = Context.IntTy;
+ BestWidth = CharWidth;
+ } else if (isPacked && NumPositiveBits <= ShortWidth) {
+ BestType = Context.UnsignedShortTy;
+ BestPromotionType = Context.IntTy;
+ BestWidth = ShortWidth;
+ } else if (NumPositiveBits <= IntWidth) {
+ BestType = Context.UnsignedIntTy;
+ BestWidth = IntWidth;
+ BestPromotionType = (NumPositiveBits == BestWidth || !is_cpp)
+ ? Context.UnsignedIntTy
+ : Context.IntTy;
+ } else if (NumPositiveBits <=
+ (BestWidth = Context.getTargetInfo().getLongWidth())) {
+ BestType = Context.UnsignedLongTy;
+ BestPromotionType = (NumPositiveBits == BestWidth || !is_cpp)
+ ? Context.UnsignedLongTy
+ : Context.LongTy;
+ } else {
+ BestWidth = Context.getTargetInfo().getLongLongWidth();
+ if (NumPositiveBits > BestWidth) {
+ // This can happen with bit-precise integer types, but those are not
+ // allowed as the type for an enumerator per C23 6.7.2.2p4 and p12.
+ // FIXME: GCC uses __int128_t and __uint128_t for cases that fit within
+ // a 128-bit integer, we should consider doing the same.
+ enum_too_large = true;
+ }
+ BestType = Context.UnsignedLongLongTy;
+ BestPromotionType = (NumPositiveBits == BestWidth || !is_cpp)
+ ? Context.UnsignedLongLongTy
+ : Context.LongLongTy;
+ }
+ }
+ return enum_too_large;
+}
+
void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
Decl *EnumDeclX, ArrayRef<Decl *> Elements, Scope *S,
const ParsedAttributesView &Attrs) {
@@ -20030,10 +20111,6 @@ void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
return;
}
- unsigned IntWidth = Context.getTargetInfo().getIntWidth();
- unsigned CharWidth = Context.getTargetInfo().getCharWidth();
- unsigned ShortWidth = Context.getTargetInfo().getShortWidth();
-
// Verify that all the values are okay, compute the size of the values, and
// reverse the list.
unsigned NumNegativeBits = 0;
@@ -20099,73 +20176,12 @@ void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
BestPromotionType = BestType;
BestWidth = Context.getIntWidth(BestType);
- }
- else if (NumNegativeBits) {
- // If there is a negative value, figure out the smallest integer type (of
- // int/long/longlong) that fits.
- // If it's packed, check also if it fits a char or a short.
- if (Packed && NumNegativeBits <= CharWidth && NumPositiveBits < CharWidth) {
- BestType = Context.SignedCharTy;
- BestWidth = CharWidth;
- } else if (Packed && NumNegativeBits <= ShortWidth &&
- NumPositiveBits < ShortWidth) {
- BestType = Context.ShortTy;
- BestWidth = ShortWidth;
- } else if (NumNegativeBits <= IntWidth && NumPositiveBits < IntWidth) {
- BestType = Context.IntTy;
- BestWidth = IntWidth;
- } else {
- BestWidth = Context.getTargetInfo().getLongWidth();
-
- if (NumNegativeBits <= BestWidth && NumPositiveBits < BestWidth) {
- BestType = Context.LongTy;
- } else {
- BestWidth = Context.getTargetInfo().getLongLongWidth();
-
- if (NumNegativeBits > BestWidth || NumPositiveBits >= BestWidth)
- Diag(Enum->getLocation(), diag::ext_enum_too_large);
- BestType = Context.LongLongTy;
- }
- }
- BestPromotionType = (BestWidth <= IntWidth ? Context.IntTy : BestType);
} else {
- // If there is no negative value, figure out the smallest type that fits
- // all of the enumerator values.
- // If it's packed, check also if it fits a char or a short.
- if (Packed && NumPositiveBits <= CharWidth) {
- BestType = Context.UnsignedCharTy;
- BestPromotionType = Context.IntTy;
- BestWidth = CharWidth;
- } else if (Packed && NumPositiveBits <= ShortWidth) {
- BestType = Context.UnsignedShortTy;
- BestPromotionType = Context.IntTy;
- BestWidth = ShortWidth;
- } else if (NumPositiveBits <= IntWidth) {
- BestType = Context.UnsignedIntTy;
- BestWidth = IntWidth;
- BestPromotionType
- = (NumPositiveBits == BestWidth || !getLangOpts().CPlusPlus)
- ? Context.UnsignedIntTy : Context.IntTy;
- } else if (NumPositiveBits <=
- (BestWidth = Context.getTargetInfo().getLongWidth())) {
- BestType = Context.UnsignedLongTy;
- BestPromotionType
- = (NumPositiveBits == BestWidth || !getLangOpts().CPlusPlus)
- ? Context.UnsignedLongTy : Context.LongTy;
- } else {
- BestWidth = Context.getTargetInfo().getLongLongWidth();
- if (NumPositiveBits > BestWidth) {
- // This can happen with bit-precise integer types, but those are not
- // allowed as the type for an enumerator per C23 6.7.2.2p4 and p12.
- // FIXME: GCC uses __int128_t and __uint128_t for cases that fit within
- // a 128-bit integer, we should consider doing the same.
- Diag(Enum->getLocation(), diag::ext_enum_too_large);
- }
- BestType = Context.UnsignedLongLongTy;
- BestPromotionType
- = (NumPositiveBits == BestWidth || !getLangOpts().CPlusPlus)
- ? Context.UnsignedLongLongTy : Context.LongLongTy;
- }
+ bool enum_too_large = ComputeBestEnumProperties(
+ Context, Enum, getLangOpts().CPlusPlus, Packed, NumNegativeBits,
+ NumPositiveBits, BestWidth, BestType, BestPromotionType);
+ if (enum_too_large)
+ Diag(Enum->getLocation(), diag::ext_enum_too_large);
}
// Loop over all of the enumerator constants, changing their types to match
@@ -20197,7 +20213,7 @@ void Sema::ActOnEnumBody(SourceLocation EnumLoc, SourceRange BraceRange,
// int; or,
// - the enumerated type
NewTy = Context.IntTy;
- NewWidth = IntWidth;
+ NewWidth = Context.getTargetInfo().getIntWidth();
NewSign = true;
} else if (ECD->getType() == BestType) {
// Already the right type!
|
Fznamznon
reviewed
Dec 23, 2024
clang/lib/Sema/SemaDecl.cpp
Outdated
Comment on lines
20011
to
20012
| bool Sema::ComputeBestEnumProperties(ASTContext &Context, EnumDecl *Enum, | ||
| bool is_cpp, bool isPacked, |
There was a problem hiding this comment.
Unless I'm missing something, Enum parameter is actually unused.
Also, is_cpp can be queried from ASTContext, via Context.getLangOpts().CPlusPlus.
Context parameter is not required, since it can be queried from Sema itself via simply using Context.
I would expect that BestWidth is width of the BestType, right? It probably can be simply queried via ASTContext::getIntWidth(BestType) instead of passing to/from a function.
There was a problem hiding this comment.
Removed Enum, is_cpp and BestWidth, but unfortunately when calling this function from LLDB, Context in Sema is empty, so it has to be passed as an argument.
There was a problem hiding this comment.
Hmm, then I wonder if it makes sense to make it a method of ASTContext instead of method of Sema? Usually ASTContext is used to query type information, so it might make sense.
There was a problem hiding this comment.
Maybe better to put this function into EnumDecl itself? It has ASTContext within it filled on both Sema and LLDB side, and then EnumDecl can fill it's own BestType and BestPromotionType fields using this function.
There was a problem hiding this comment.
EnumDecl can fill it's own BestType and BestPromotionType fields using this function.
I'm afraid it might result in EnumDecl taking more space and I don't think we'd like that.
There was a problem hiding this comment.
EnumDecl can fill it's own BestType and BestPromotionType fields using this function.
I'm afraid it might result in EnumDecl taking more space and I don't think we'd like that.
I'm sorry, I'm a bit confused here, what kind of space do you mean?
I suggest adding only one method to the class, nothing else.
There was a problem hiding this comment.
I suggest adding only one method to the class, nothing else.
Oh, I'm sorry, I thought you meant adding more fields to EnumDecl to save BestType and BestPromotionType. Since there can be quite a lot of AST nodes including EnumDecl that might affect memory consumption of clang in a negative way.
If I got it wrong, then it is probably fine I think.
There was a problem hiding this comment.
Moving the function into EnumDecl would probably work, but considering how many occurences of Context. there are in the function, it feels more like it really does belong in ASTContext.
clang/lib/Sema/SemaDecl.cpp
Outdated
| = (NumPositiveBits == BestWidth || !getLangOpts().CPlusPlus) | ||
| ? Context.UnsignedLongLongTy : Context.LongLongTy; | ||
| } | ||
| bool enum_too_large = ComputeBestEnumProperties( |
There was a problem hiding this comment.
Please follow the codestyle when possible
Suggested change
| bool enum_too_large = ComputeBestEnumProperties( | |
| bool EnumTooLarge = ComputeBestEnumProperties( |
kuilpd
force-pushed
the
clang-move-enum-width-type-separate
branch
from
87fb09f to
c41ddb9
Compare
Michael137
reviewed
Jan 2, 2025
clang/lib/Sema/SemaDecl.cpp
Outdated
| = (NumPositiveBits == BestWidth || !getLangOpts().CPlusPlus) | ||
| ? Context.UnsignedLongLongTy : Context.LongLongTy; | ||
| } | ||
| bool EnumTooLarge = |
There was a problem hiding this comment.
Suggested change
| bool EnumTooLarge = | |
| const bool EnumTooLarge = |
There was a problem hiding this comment.
Afaik we normally don’t do top-level const on local variables in Clang.
Sirraide
reviewed
Jan 13, 2025
|
Tried moving the code to |
Honestly, just put it wherever I’d say. Unlike |
Sirraide
approved these changes
Jan 14, 2025
|
Updated code, fixed nits. |
kuilpd
changed the title
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/11900 Here is the relevant piece of the build log for the reference |
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.
Move the code that computes BestType and BestPromotionType for an enum to a separate function which can be called from outside of Sema.