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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
