[clang] [clang] Refactor TBAA Base Info construction (PR #70499)
via cfe-commits
cfe-commits at lists.llvm.org
Mon Oct 30 04:41:41 PDT 2023
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-codegen
Author: Nathan Sidwell (urnathan)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/70499.diff
3 Files Affected:
- (modified) clang/lib/CodeGen/CodeGenModule.cpp (+1-1)
- (modified) clang/lib/CodeGen/CodeGenTBAA.cpp (+39-30)
- (modified) clang/lib/CodeGen/CodeGenTBAA.h (+7-6)
``````````diff
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);
``````````
</details>
https://github.com/llvm/llvm-project/pull/70499
More information about the cfe-commits
mailing list