-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[NFC] Address bit-field storage sizes to ensure ideal packing #139825
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
Changes from all commits
a28452d
3a2b2f2
868f0a9
980a6fa
c74da90
004ff93
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -980,7 +980,8 @@ class Sema; | |
/// Have we matched any packs on the parameter side, versus any non-packs on | ||
/// the argument side, in a context where the opposite matching is also | ||
/// allowed? | ||
bool StrictPackMatch : 1; | ||
LLVM_PREFERRED_TYPE(bool) | ||
unsigned StrictPackMatch : 1; | ||
|
||
/// True if the candidate was found using ADL. | ||
LLVM_PREFERRED_TYPE(CallExpr::ADLCallKind) | ||
|
@@ -996,7 +997,8 @@ class Sema; | |
|
||
/// FailureKind - The reason why this candidate is not viable. | ||
/// Actually an OverloadFailureKind. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should I remove this comment now that there's a preferred type annotation? |
||
unsigned char FailureKind; | ||
LLVM_PREFERRED_TYPE(OverloadFailureKind) | ||
unsigned FailureKind : 8; | ||
|
||
/// The number of call arguments that were explicitly provided, | ||
/// to be used while performing partial ordering of function templates. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,7 +103,7 @@ enum class FirstCoroutineStmtKind { CoReturn, CoAwait, CoYield }; | |
/// currently being parsed. | ||
class FunctionScopeInfo { | ||
protected: | ||
enum ScopeKind { | ||
enum ScopeKind : uint8_t { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the restricted uses, making the storage type explicit here seemed like the best call |
||
SK_Function, | ||
SK_Block, | ||
SK_Lambda, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
#include "llvm/ADT/IntrusiveRefCntPtr.h" | ||
#include "llvm/ADT/StringMap.h" | ||
#include "llvm/ADT/StringRef.h" | ||
#include "llvm/Support/Compiler.h" | ||
#include <string> | ||
#include <utility> | ||
#include <vector> | ||
|
@@ -269,7 +270,8 @@ class AnalyzerOptions { | |
unsigned NoRetryExhausted : 1; | ||
|
||
/// Emit analyzer warnings as errors. | ||
bool AnalyzerWerror : 1; | ||
LLVM_PREFERRED_TYPE(bool) | ||
unsigned AnalyzerWerror : 1; | ||
|
||
/// The inlining stack depth limit. | ||
unsigned InlineMaxStackDepth; | ||
|
@@ -410,7 +412,7 @@ class AnalyzerOptions { | |
// an alias to the new verbose filename option because this | ||
// closely mimics the behavior under the old option. | ||
ShouldWriteStableReportFilename || ShouldWriteVerboseReportFilename, | ||
AnalyzerWerror, | ||
static_cast<bool>(AnalyzerWerror), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This cast is needed before clang20 as we generate a warning about narrowing AnalyzerWerror from an int1 to a bool :D |
||
ShouldApplyFixIts, | ||
ShouldDisplayCheckerNameForText}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ | |
#include "llvm/ADT/STLExtras.h" | ||
#include "llvm/ADT/SmallVector.h" | ||
#include "llvm/ADT/StringTable.h" | ||
#include "llvm/Support/Compiler.h" | ||
#include "llvm/Support/ErrorHandling.h" | ||
#include "llvm/Support/Path.h" | ||
#include <map> | ||
|
@@ -74,19 +75,21 @@ enum DiagnosticClass { | |
struct StaticDiagInfoRec { | ||
uint16_t DiagID; | ||
LLVM_PREFERRED_TYPE(diag::Severity) | ||
uint8_t DefaultSeverity : 3; | ||
uint16_t DefaultSeverity : 3; | ||
LLVM_PREFERRED_TYPE(DiagnosticClass) | ||
uint8_t Class : 3; | ||
uint16_t Class : 3; | ||
LLVM_PREFERRED_TYPE(DiagnosticIDs::SFINAEResponse) | ||
uint8_t SFINAE : 2; | ||
uint8_t Category : 6; | ||
uint16_t SFINAE : 2; | ||
LLVM_PREFERRED_TYPE(diag::DiagCategory) | ||
uint16_t Category : 6; | ||
LLVM_PREFERRED_TYPE(bool) | ||
uint8_t WarnNoWerror : 1; | ||
uint16_t WarnNoWerror : 1; | ||
LLVM_PREFERRED_TYPE(bool) | ||
uint8_t WarnShowInSystemHeader : 1; | ||
uint16_t WarnShowInSystemHeader : 1; | ||
LLVM_PREFERRED_TYPE(bool) | ||
uint8_t WarnShowInSystemMacro : 1; | ||
uint16_t WarnShowInSystemMacro : 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really wish we had some way of directly asserting the size/packing/offsets, imagine a hyopthetical static_assert(__builtin_current_offset() == x) It would make reasoning about layout significantly easier |
||
|
||
LLVM_PREFERRED_TYPE(diag::Group) | ||
uint16_t OptionGroupIndex : 15; | ||
LLVM_PREFERRED_TYPE(bool) | ||
uint16_t Deferrable : 1; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,10 +63,10 @@ class LLVM_LIBRARY_VISIBILITY CodeViewDebug : public DebugHandlerBase { | |
int DataOffset : 31; | ||
|
||
/// Non-zero if this is a piece of an aggregate. | ||
uint16_t IsSubfield : 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ideally would not be necessary as I believe this field should be 2 byte aligned here and so ms abi would not add unneeded padding, but when making this diagnostic I found that diagnosing this correctly required duplicating the record layout to essentially track the addition of otherwise unneeded padding, and I was concerned that you would get surprising behavior from otherwise "simple" changes (e.g. moving a bit-field around could suddenly unalign another field and trigger terrible padding) |
||
uint32_t IsSubfield : 1; | ||
|
||
/// Offset into aggregate. | ||
uint16_t StructOffset : 15; | ||
uint32_t StructOffset : 15; | ||
|
||
/// Register containing the data or the register base of the memory | ||
/// location containing the data. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured naming this would not hurt, as I'm updating its storage and not specifying the type made me feel it was at risk of slipping through the cracks.
I've removed the storage sizes changes I was going to make so it could just be a correctly typed field as well on the basis that once I'm done with this round of changes I may go through and start trying to reduce the use of preferred type enums