[clang] [clang] Refactor TBAA Base Info construction (PR #70499)
Nathan Sidwell via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 27 12:19:08 PDT 2023
https://github.com/urnathan created https://github.com/llvm/llvm-project/pull/70499
I noticed a few issues with `CodeGenTBAA::getBaseTypeInfo`.
1) `isValidBaseType` explicitly checks for a reference type to return false, but then also returns false for all non-record types. Just remove that reference check.
2) All uses of `CodeGenTBAA::getBaseTypeInfo` from within that class are when we've already checked the type `isValidBaseType`. The only case where this isn't true is from outside the class. `isValidBaseType` isn't a trivial check, so add a new `maybeGetBaseTypeInfo` entry point that returns nullptr for non-valid base types, and remove the check from the current entry point. Make that private too.
3) The `BaseTypeMetadataCache` can legitimately store null values, but it also uses that to mean 'no entry', because of the use of `operator[]`. Hence it can continually recompute the metadata information for those types with null metadata. (AFAICT null entries can never become non-null.) Use `find` to lookup, and only compute when there was no entry.
4) Both `getBaseTypeInfo` and `getBaseTypeInfoHelper` insert the metadata into the cache -- but the latter does so inconsistently. So only do the insertion in the former, and use `try_emplace` to insert and verify it didn't get unexpectedly inserted during its own calculation. (And side-effecting return statements make me uncomfortable.)
5) Finally, `getBaseTypeInfoHelper` is rather tightly tied to `getBaseTypeInfo`, its only caller. It's more localized to use a lambda there, and will make it easier to be inlined into its only caller.
I tried getting some performance data with bootstrap builds. While some runs showed an improvement (.3%), there seemed to be quite a bit of noise, and .3% seems surprisingly high.
>From 43d816c2459e904d982453d91473ac2a04773ee7 Mon Sep 17 00:00:00 2001
From: Nathan Sidwell <nathan at acm.org>
Date: Tue, 24 Oct 2023 20:22:45 -0400
Subject: [PATCH] [clang] Refactor TBAA Base Info construction
I noticed a few issues with CodeGenTBAA::getBaseTypeInfo.
1) isValidBaseType explicitly checks for a reference type to return
false, but then also returns false for all non-record types. Just
remove that reference check.
2) All uses of CodeGenTBAA::getBaseTypeInfo from within that class are
when we've already checked the type isValidBaseType. The only case
where this isn't true is from outside the class. isValidBaseType isn't
a trivial check, so add a new 'maybeGetBaseTypeInfo' entry point that
returns nullptr for non valid base types, and remove the check from
the current entry point. Make that private too.
3) The BaseTypeMetadataCache can legitimately store null values, but
also use that to mean 'no entry', and hence can continually recompute
the metadata information for those types with null metadata. (AFAICT
null entries can never become non-null.) Use find to lookup, and only
compute when there was no entry.
4) Both getBaseTypeInfo and getBaseTypeInfoHelper insert the metadata
into the cache -- but the latter does so inconsistently. So only
insert in the former, and use try_emplace to insert and verify it
didn't get unexpectedly inserted during its own calculation. (And
side-effecting 'return' statements make me uncomfortable.)
5) Finally, getBaseTypeInfoHelper is rather tightly tied to
getBaseTypeInfo, its only caller. It's more localized to use a lambda
there, and will make it easier to be inlined into its only caller.
---
clang/lib/CodeGen/CodeGenModule.cpp | 2 +-
clang/lib/CodeGen/CodeGenTBAA.cpp | 69 ++++++++++++++++-------------
clang/lib/CodeGen/CodeGenTBAA.h | 13 +++---
3 files changed, 47 insertions(+), 37 deletions(-)
diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index b1a6683a66bd052..de78a3bbd4f78a6 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -1309,7 +1309,7 @@ llvm::MDNode *CodeGenModule::getTBAAStructInfo(QualType QTy) {
llvm::MDNode *CodeGenModule::getTBAABaseTypeInfo(QualType QTy) {
if (!TBAA)
return nullptr;
- return TBAA->getBaseTypeInfo(QTy);
+ return TBAA->maybeGetBaseTypeInfo(QTy);
}
llvm::MDNode *CodeGenModule::getTBAAAccessTagInfo(TBAAAccessInfo Info) {
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp b/clang/lib/CodeGen/CodeGenTBAA.cpp
index 8705d3d65f1a573..c48f72ba2b9ccb8 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -95,8 +95,6 @@ static bool TypeHasMayAlias(QualType QTy) {
/// Check if the given type is a valid base type to be used in access tags.
static bool isValidBaseType(QualType QTy) {
- if (QTy->isReferenceType())
- return false;
if (const RecordType *TTy = QTy->getAs<RecordType>()) {
const RecordDecl *RD = TTy->getDecl()->getDefinition();
// Incomplete types are not valid base access types.
@@ -331,8 +329,20 @@ CodeGenTBAA::getTBAAStructInfo(QualType QTy) {
return StructMetadataCache[Ty] = nullptr;
}
-llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
- if (auto *TTy = dyn_cast<RecordType>(Ty)) {
+llvm::MDNode *CodeGenTBAA::getBaseTypeInfo(QualType QTy) {
+ assert(isValidBaseType(QTy) && "Type is not a valid base type");
+
+ const RecordType *Ty =
+ cast<RecordType>(Context.getCanonicalType(QTy).getTypePtr());
+
+ // null is a valid value in the cache.
+ auto I = BaseTypeMetadataCache.find(Ty);
+ if (I != BaseTypeMetadataCache.end())
+ return I->second;
+
+ // This lambda can recursively call us, so may add new nodes to the cache, and
+ // hence invalidate previously obtained iterators.
+ auto ComputeBaseTypeInfo = [&](const RecordType *TTy) -> llvm::MDNode * {
const RecordDecl *RD = TTy->getDecl()->getDefinition();
const ASTRecordLayout &Layout = Context.getASTRecordLayout(RD);
using TBAAStructField = llvm::MDBuilder::TBAAStructField;
@@ -342,7 +352,7 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
// field. Virtual bases are more complex and omitted, but avoid an
// incomplete view for NewStructPathTBAA.
if (CodeGenOpts.NewStructPathTBAA && CXXRD->getNumVBases() != 0)
- return BaseTypeMetadataCache[Ty] = nullptr;
+ return nullptr;
for (const CXXBaseSpecifier &B : CXXRD->bases()) {
if (B.isVirtual())
continue;
@@ -354,7 +364,7 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
? getBaseTypeInfo(BaseQTy)
: getTypeInfo(BaseQTy);
if (!TypeNode)
- return BaseTypeMetadataCache[Ty] = nullptr;
+ return nullptr;
uint64_t Offset = Layout.getBaseClassOffset(BaseRD).getQuantity();
uint64_t Size =
Context.getASTRecordLayout(BaseRD).getDataSize().getQuantity();
@@ -363,9 +373,8 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
}
// The order in which base class subobjects are allocated is unspecified,
// so may differ from declaration order. In particular, Itanium ABI will
- // allocate a primary base first.
- // Since we exclude empty subobjects, the objects are not overlapping and
- // their offsets are unique.
+ // allocate a primary base first. Since we exclude empty subobjects, the
+ // objects are not overlapping and their offsets are unique.
llvm::sort(Fields,
[](const TBAAStructField &A, const TBAAStructField &B) {
return A.Offset < B.Offset;
@@ -375,57 +384,57 @@ llvm::MDNode *CodeGenTBAA::getBaseTypeInfoHelper(const Type *Ty) {
if (Field->isZeroSize(Context) || Field->isUnnamedBitfield())
continue;
QualType FieldQTy = Field->getType();
- llvm::MDNode *TypeNode = isValidBaseType(FieldQTy) ?
- getBaseTypeInfo(FieldQTy) : getTypeInfo(FieldQTy);
+ llvm::MDNode *TypeNode = isValidBaseType(FieldQTy)
+ ? getBaseTypeInfo(FieldQTy)
+ : getTypeInfo(FieldQTy);
if (!TypeNode)
- return BaseTypeMetadataCache[Ty] = nullptr;
+ return nullptr;
uint64_t BitOffset = Layout.getFieldOffset(Field->getFieldIndex());
uint64_t Offset = Context.toCharUnitsFromBits(BitOffset).getQuantity();
uint64_t Size = Context.getTypeSizeInChars(FieldQTy).getQuantity();
- Fields.push_back(llvm::MDBuilder::TBAAStructField(Offset, Size,
- TypeNode));
+ Fields.push_back(
+ llvm::MDBuilder::TBAAStructField(Offset, Size, TypeNode));
}
SmallString<256> OutName;
if (Features.CPlusPlus) {
// Don't use the mangler for C code.
llvm::raw_svector_ostream Out(OutName);
- MContext.mangleCanonicalTypeName(QualType(Ty, 0), Out);
+ MContext.mangleCanonicalTypeName(QualType(TTy, 0), Out);
} else {
OutName = RD->getName();
}
if (CodeGenOpts.NewStructPathTBAA) {
llvm::MDNode *Parent = getChar();
- uint64_t Size = Context.getTypeSizeInChars(Ty).getQuantity();
+ uint64_t Size = Context.getTypeSizeInChars(TTy).getQuantity();
llvm::Metadata *Id = MDHelper.createString(OutName);
return MDHelper.createTBAATypeNode(Parent, Size, Id, Fields);
}
// Create the struct type node with a vector of pairs (offset, type).
- SmallVector<std::pair<llvm::MDNode*, uint64_t>, 4> OffsetsAndTypes;
+ SmallVector<std::pair<llvm::MDNode *, uint64_t>, 4> OffsetsAndTypes;
for (const auto &Field : Fields)
- OffsetsAndTypes.push_back(std::make_pair(Field.Type, Field.Offset));
+ OffsetsAndTypes.push_back(std::make_pair(Field.Type, Field.Offset));
return MDHelper.createTBAAStructTypeNode(OutName, OffsetsAndTypes);
- }
+ };
+
+ // First calculate the metadata, before recomputing the insertion point, as
+ // the helper can recursively call us.
+ llvm::MDNode *TypeNode = ComputeBaseTypeInfo(Ty);
+ LLVM_ATTRIBUTE_UNUSED auto inserted =
+ BaseTypeMetadataCache.try_emplace(Ty, TypeNode);
+ assert(inserted.second && "BaseType metadata was already inserted");
- return nullptr;
+ return TypeNode;
}
-llvm::MDNode *CodeGenTBAA::getBaseTypeInfo(QualType QTy) {
+llvm::MDNode *CodeGenTBAA::maybeGetBaseTypeInfo(QualType QTy) {
if (!isValidBaseType(QTy))
return nullptr;
- const Type *Ty = Context.getCanonicalType(QTy).getTypePtr();
- if (llvm::MDNode *N = BaseTypeMetadataCache[Ty])
- return N;
-
- // Note that the following helper call is allowed to add new nodes to the
- // cache, which invalidates all its previously obtained iterators. So we
- // first generate the node for the type and then add that node to the cache.
- llvm::MDNode *TypeNode = getBaseTypeInfoHelper(Ty);
- return BaseTypeMetadataCache[Ty] = TypeNode;
+ return getBaseTypeInfo(QTy);
}
llvm::MDNode *CodeGenTBAA::getAccessTagInfo(TBAAAccessInfo Info) {
diff --git a/clang/lib/CodeGen/CodeGenTBAA.h b/clang/lib/CodeGen/CodeGenTBAA.h
index a65963596fe9def..184fbd5573be941 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.h
+++ b/clang/lib/CodeGen/CodeGenTBAA.h
@@ -162,9 +162,9 @@ class CodeGenTBAA {
/// to describe accesses to objects of the given type.
llvm::MDNode *getTypeInfoHelper(const Type *Ty);
- /// getBaseTypeInfoHelper - An internal helper function to generate metadata
- /// used to describe accesses to objects of the given base type.
- llvm::MDNode *getBaseTypeInfoHelper(const Type *Ty);
+ /// getBaseTypeInfo - Lazily compute metadata that describes the given base
+ /// access type. The type must be suitable.
+ llvm::MDNode *getBaseTypeInfo(QualType QTy);
public:
CodeGenTBAA(ASTContext &Ctx, llvm::Module &M, const CodeGenOptions &CGO,
@@ -187,9 +187,10 @@ class CodeGenTBAA {
/// the given type.
llvm::MDNode *getTBAAStructInfo(QualType QTy);
- /// getBaseTypeInfo - Get metadata that describes the given base access type.
- /// Return null if the type is not suitable for use in TBAA access tags.
- llvm::MDNode *getBaseTypeInfo(QualType QTy);
+ /// maybeGetBaseTypeInfo - Get metadata that describes the given base access
+ /// type. Return null if the type is not suitable for use in TBAA access
+ /// tags.
+ llvm::MDNode *maybeGetBaseTypeInfo(QualType QTy);
/// getAccessTagInfo - Get TBAA tag for a given memory access.
llvm::MDNode *getAccessTagInfo(TBAAAccessInfo Info);
More information about the cfe-commits
mailing list