From 4e321bb77ca6cb716bfdbc4141de73d3ad77443d Mon Sep 17 00:00:00 2001 From: Doug Wyatt Date: Wed, 17 Jul 2024 10:36:36 -0700 Subject: [PATCH] Performance optimizations for function effects (nonblocking attribute etc.) (#96844) Summary: - Put new FunctionProtoType trailing objects last. - Inline FunctionEffectsRef::get() - Manually inline FunctionEffectsRef::Profile(). --------- Co-authored-by: Doug Wyatt Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250936 --- clang/include/clang/AST/ASTContext.h | 5 ++ clang/include/clang/AST/Type.h | 32 ++++++++---- clang/lib/AST/ASTContext.cpp | 12 +++-- clang/lib/AST/Type.cpp | 36 +++++--------- clang/lib/Sema/Sema.cpp | 4 +- clang/lib/Sema/SemaDecl.cpp | 74 ++++++++++++++-------------- clang/lib/Sema/SemaDeclCXX.cpp | 62 ++++++++++++----------- clang/lib/Sema/SemaOverload.cpp | 2 +- 8 files changed, 119 insertions(+), 108 deletions(-) diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index 13aa203de32ba9..608bd90fcc3ff9 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -643,6 +643,9 @@ class ASTContext : public RefCountedBase { /// address spaces (e.g. OpenCL/CUDA) bool AddrSpaceMapMangling; + /// For performance, track whether any function effects are in use. + mutable bool AnyFunctionEffects = false; + const TargetInfo *Target = nullptr; const TargetInfo *AuxTarget = nullptr; clang::PrintingPolicy PrintingPolicy; @@ -2909,6 +2912,8 @@ class ASTContext : public RefCountedBase { return AddrSpaceMapMangling || isTargetAddressSpace(AS); } + bool hasAnyFunctionEffects() const { return AnyFunctionEffects; } + // Merges two exception specifications, such that the resulting // exception spec is the union of both. For example, if either // of them can throw something, the result can throw it as well. diff --git a/clang/include/clang/AST/Type.h b/clang/include/clang/AST/Type.h index 4c9ba37fe1e3a2..25defea58c2dc2 100644 --- a/clang/include/clang/AST/Type.h +++ b/clang/include/clang/AST/Type.h @@ -132,7 +132,6 @@ class TemplateArgument; class TemplateArgumentListInfo; class TemplateArgumentLoc; class TemplateTypeParmDecl; -template class TreeTransform; class TypedefNameDecl; class UnresolvedUsingTypenameDecl; class UsingShadowDecl; @@ -4901,7 +4900,6 @@ class FunctionEffectsRef { return !(LHS == RHS); } - void Profile(llvm::FoldingSetNodeID &ID) const; void dump(llvm::raw_ostream &OS) const; }; @@ -4971,8 +4969,8 @@ class FunctionProtoType final FunctionProtoType, QualType, SourceLocation, FunctionType::FunctionTypeExtraBitfields, FunctionType::FunctionTypeArmAttributes, FunctionType::ExceptionType, - Expr *, FunctionDecl *, FunctionType::ExtParameterInfo, - FunctionEffect, EffectConditionExpr, Qualifiers> { + Expr *, FunctionDecl *, FunctionType::ExtParameterInfo, Qualifiers, + FunctionEffect, EffectConditionExpr> { friend class ASTContext; // ASTContext creates these. friend TrailingObjects; @@ -5003,21 +5001,21 @@ class FunctionProtoType final // an ExtParameterInfo for each of the parameters. Present if and // only if hasExtParameterInfos() is true. // + // * Optionally a Qualifiers object to represent extra qualifiers that can't + // be represented by FunctionTypeBitfields.FastTypeQuals. Present if and + // only if hasExtQualifiers() is true. + // // * Optionally, an array of getNumFunctionEffects() FunctionEffect. // Present only when getNumFunctionEffects() > 0 // // * Optionally, an array of getNumFunctionEffects() EffectConditionExpr. // Present only when getNumFunctionEffectConditions() > 0. // - // * Optionally a Qualifiers object to represent extra qualifiers that can't - // be represented by FunctionTypeBitfields.FastTypeQuals. Present if and - // only if hasExtQualifiers() is true. - // // The optional FunctionTypeExtraBitfields has to be before the data // related to the exception specification since it contains the number // of exception types. // - // We put the ExtParameterInfos last. If all were equal, it would make + // We put the ExtParameterInfos later. If all were equal, it would make // more sense to put these before the exception specification, because // it's much easier to skip past them compared to the elaborate switch // required to skip the exception specification. However, all is not @@ -5134,6 +5132,10 @@ class FunctionProtoType final return hasExtParameterInfos() ? getNumParams() : 0; } + unsigned numTrailingObjects(OverloadToken) const { + return hasExtQualifiers() ? 1 : 0; + } + unsigned numTrailingObjects(OverloadToken) const { return getNumFunctionEffects(); } @@ -8616,6 +8618,18 @@ QualType DecayedType::getPointeeType() const { void FixedPointValueToString(SmallVectorImpl &Str, llvm::APSInt Val, unsigned Scale); +inline FunctionEffectsRef FunctionEffectsRef::get(QualType QT) { + while (true) { + QualType Pointee = QT->getPointeeType(); + if (Pointee.isNull()) + break; + QT = Pointee; + } + if (const auto *FPT = QT->getAs()) + return FPT->getFunctionEffects(); + return {}; +} + } // namespace clang #endif // LLVM_CLANG_AST_TYPE_H diff --git a/clang/lib/AST/ASTContext.cpp b/clang/lib/AST/ASTContext.cpp index f4aa1387974aa0..a8e599f7ebe049 100644 --- a/clang/lib/AST/ASTContext.cpp +++ b/clang/lib/AST/ASTContext.cpp @@ -4896,14 +4896,14 @@ QualType ASTContext::getFunctionTypeInternal( size_t Size = FunctionProtoType::totalSizeToAlloc< QualType, SourceLocation, FunctionType::FunctionTypeExtraBitfields, FunctionType::FunctionTypeArmAttributes, FunctionType::ExceptionType, - Expr *, FunctionDecl *, FunctionProtoType::ExtParameterInfo, - FunctionEffect, EffectConditionExpr, Qualifiers>( + Expr *, FunctionDecl *, FunctionProtoType::ExtParameterInfo, Qualifiers, + FunctionEffect, EffectConditionExpr>( NumArgs, EPI.Variadic, EPI.requiresFunctionProtoTypeExtraBitfields(), EPI.requiresFunctionProtoTypeArmAttributes(), ESH.NumExceptionType, ESH.NumExprPtr, ESH.NumFunctionDeclPtr, - EPI.ExtParameterInfos ? NumArgs : 0, EPI.FunctionEffects.size(), - EPI.FunctionEffects.conditions().size(), - EPI.TypeQuals.hasNonFastQualifiers() ? 1 : 0); + EPI.ExtParameterInfos ? NumArgs : 0, + EPI.TypeQuals.hasNonFastQualifiers() ? 1 : 0, EPI.FunctionEffects.size(), + EPI.FunctionEffects.conditions().size()); auto *FTP = (FunctionProtoType *)Allocate(Size, alignof(FunctionProtoType)); FunctionProtoType::ExtProtoInfo newEPI = EPI; @@ -4911,6 +4911,8 @@ QualType ASTContext::getFunctionTypeInternal( Types.push_back(FTP); if (!Unique) FunctionProtoTypes.InsertNode(FTP, InsertPos); + if (!EPI.FunctionEffects.empty()) + AnyFunctionEffects = true; return QualType(FTP, 0); } diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp index 5bf1f3dbdbd4b3..fdaab8e4345936 100644 --- a/clang/lib/AST/Type.cpp +++ b/clang/lib/AST/Type.cpp @@ -3798,9 +3798,18 @@ void FunctionProtoType::Profile(llvm::FoldingSetNodeID &ID, QualType Result, } epi.ExtInfo.Profile(ID); - ID.AddInteger((epi.AArch64SMEAttributes << 1) | epi.HasTrailingReturn); - epi.FunctionEffects.Profile(ID); + unsigned EffectCount = epi.FunctionEffects.size(); + bool HasConds = !epi.FunctionEffects.Conditions.empty(); + + ID.AddInteger((EffectCount << 3) | (HasConds << 2) | + (epi.AArch64SMEAttributes << 1) | epi.HasTrailingReturn); + + for (unsigned Idx = 0; Idx != EffectCount; ++Idx) { + ID.AddInteger(epi.FunctionEffects.Effects[Idx].toOpaqueInt32()); + if (HasConds) + ID.AddPointer(epi.FunctionEffects.Conditions[Idx].getCondition()); + } } void FunctionProtoType::Profile(llvm::FoldingSetNodeID &ID, @@ -5181,17 +5190,6 @@ bool FunctionEffect::shouldDiagnoseFunctionCall( // ===== -void FunctionEffectsRef::Profile(llvm::FoldingSetNodeID &ID) const { - bool HasConds = !Conditions.empty(); - - ID.AddInteger(size() | (HasConds << 31u)); - for (unsigned Idx = 0, Count = Effects.size(); Idx != Count; ++Idx) { - ID.AddInteger(Effects[Idx].toOpaqueInt32()); - if (HasConds) - ID.AddPointer(Conditions[Idx].getCondition()); - } -} - bool FunctionEffectSet::insert(const FunctionEffectWithCondition &NewEC, Conflicts &Errs) { FunctionEffect::Kind NewOppositeKind = NewEC.Effect.oppositeKind(); @@ -5313,18 +5311,6 @@ LLVM_DUMP_METHOD void FunctionEffectSet::dump(llvm::raw_ostream &OS) const { FunctionEffectsRef(*this).dump(OS); } -FunctionEffectsRef FunctionEffectsRef::get(QualType QT) { - while (true) { - QualType Pointee = QT->getPointeeType(); - if (Pointee.isNull()) - break; - QT = Pointee; - } - if (const auto *FPT = QT->getAs()) - return FPT->getFunctionEffects(); - return {}; -} - FunctionEffectsRef FunctionEffectsRef::create(ArrayRef FX, ArrayRef Conds) { diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index d6228718d53aed..46417964f08960 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -718,8 +718,8 @@ ExprResult Sema::ImpCastExprToType(Expr *E, QualType Ty, diagnoseNullableToNonnullConversion(Ty, E->getType(), E->getBeginLoc()); diagnoseZeroToNullptrConversion(Kind, E); - if (!isCast(CCK) && Kind != CK_NullToPointer && - Kind != CK_NullToMemberPointer) + if (Context.hasAnyFunctionEffects() && !isCast(CCK) && + Kind != CK_NullToPointer && Kind != CK_NullToMemberPointer) diagnoseFunctionEffectConversion(Ty, E->getType(), E->getBeginLoc()); QualType ExprTy = Context.getCanonicalType(E->getType()); diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 1f2fde12c9d243..a3dd5ede9116a0 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -3813,45 +3813,47 @@ bool Sema::MergeFunctionDecl(FunctionDecl *New, NamedDecl *&OldD, Scope *S, return true; } - const auto OldFX = Old->getFunctionEffects(); - const auto NewFX = New->getFunctionEffects(); QualType OldQTypeForComparison = OldQType; - if (OldFX != NewFX) { - const auto Diffs = FunctionEffectDifferences(OldFX, NewFX); - for (const auto &Diff : Diffs) { - if (Diff.shouldDiagnoseRedeclaration(*Old, OldFX, *New, NewFX)) { - Diag(New->getLocation(), - diag::warn_mismatched_func_effect_redeclaration) - << Diff.effectName(); - Diag(Old->getLocation(), diag::note_previous_declaration); + if (Context.hasAnyFunctionEffects()) { + const auto OldFX = Old->getFunctionEffects(); + const auto NewFX = New->getFunctionEffects(); + if (OldFX != NewFX) { + const auto Diffs = FunctionEffectDifferences(OldFX, NewFX); + for (const auto &Diff : Diffs) { + if (Diff.shouldDiagnoseRedeclaration(*Old, OldFX, *New, NewFX)) { + Diag(New->getLocation(), + diag::warn_mismatched_func_effect_redeclaration) + << Diff.effectName(); + Diag(Old->getLocation(), diag::note_previous_declaration); + } } - } - // Following a warning, we could skip merging effects from the previous - // declaration, but that would trigger an additional "conflicting types" - // error. - if (const auto *NewFPT = NewQType->getAs()) { - FunctionEffectSet::Conflicts MergeErrs; - FunctionEffectSet MergedFX = - FunctionEffectSet::getUnion(OldFX, NewFX, MergeErrs); - if (!MergeErrs.empty()) - diagnoseFunctionEffectMergeConflicts(MergeErrs, New->getLocation(), - Old->getLocation()); - - FunctionProtoType::ExtProtoInfo EPI = NewFPT->getExtProtoInfo(); - EPI.FunctionEffects = FunctionEffectsRef(MergedFX); - QualType ModQT = Context.getFunctionType(NewFPT->getReturnType(), - NewFPT->getParamTypes(), EPI); - - New->setType(ModQT); - NewQType = New->getType(); - - // Revise OldQTForComparison to include the merged effects, - // so as not to fail due to differences later. - if (const auto *OldFPT = OldQType->getAs()) { - EPI = OldFPT->getExtProtoInfo(); + // Following a warning, we could skip merging effects from the previous + // declaration, but that would trigger an additional "conflicting types" + // error. + if (const auto *NewFPT = NewQType->getAs()) { + FunctionEffectSet::Conflicts MergeErrs; + FunctionEffectSet MergedFX = + FunctionEffectSet::getUnion(OldFX, NewFX, MergeErrs); + if (!MergeErrs.empty()) + diagnoseFunctionEffectMergeConflicts(MergeErrs, New->getLocation(), + Old->getLocation()); + + FunctionProtoType::ExtProtoInfo EPI = NewFPT->getExtProtoInfo(); EPI.FunctionEffects = FunctionEffectsRef(MergedFX); - OldQTypeForComparison = Context.getFunctionType( - OldFPT->getReturnType(), OldFPT->getParamTypes(), EPI); + QualType ModQT = Context.getFunctionType(NewFPT->getReturnType(), + NewFPT->getParamTypes(), EPI); + + New->setType(ModQT); + NewQType = New->getType(); + + // Revise OldQTForComparison to include the merged effects, + // so as not to fail due to differences later. + if (const auto *OldFPT = OldQType->getAs()) { + EPI = OldFPT->getExtProtoInfo(); + EPI.FunctionEffects = FunctionEffectsRef(MergedFX); + OldQTypeForComparison = Context.getFunctionType( + OldFPT->getReturnType(), OldFPT->getParamTypes(), EPI); + } } } } diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 2bfb103e8953d6..04b8d88cae217b 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -18100,38 +18100,40 @@ bool Sema::CheckOverridingFunctionAttributes(CXXMethodDecl *New, } // Virtual overrides: check for matching effects. - const auto OldFX = Old->getFunctionEffects(); - const auto NewFXOrig = New->getFunctionEffects(); - - if (OldFX != NewFXOrig) { - FunctionEffectSet NewFX(NewFXOrig); - const auto Diffs = FunctionEffectDifferences(OldFX, NewFX); - FunctionEffectSet::Conflicts Errs; - for (const auto &Diff : Diffs) { - switch (Diff.shouldDiagnoseMethodOverride(*Old, OldFX, *New, NewFX)) { - case FunctionEffectDiff::OverrideResult::NoAction: - break; - case FunctionEffectDiff::OverrideResult::Warn: - Diag(New->getLocation(), diag::warn_mismatched_func_effect_override) - << Diff.effectName(); - Diag(Old->getLocation(), diag::note_overridden_virtual_function) - << Old->getReturnTypeSourceRange(); - break; - case FunctionEffectDiff::OverrideResult::Merge: { - NewFX.insert(Diff.Old, Errs); - const auto *NewFT = New->getType()->castAs(); - FunctionProtoType::ExtProtoInfo EPI = NewFT->getExtProtoInfo(); - EPI.FunctionEffects = FunctionEffectsRef(NewFX); - QualType ModQT = Context.getFunctionType(NewFT->getReturnType(), - NewFT->getParamTypes(), EPI); - New->setType(ModQT); - break; - } + if (Context.hasAnyFunctionEffects()) { + const auto OldFX = Old->getFunctionEffects(); + const auto NewFXOrig = New->getFunctionEffects(); + + if (OldFX != NewFXOrig) { + FunctionEffectSet NewFX(NewFXOrig); + const auto Diffs = FunctionEffectDifferences(OldFX, NewFX); + FunctionEffectSet::Conflicts Errs; + for (const auto &Diff : Diffs) { + switch (Diff.shouldDiagnoseMethodOverride(*Old, OldFX, *New, NewFX)) { + case FunctionEffectDiff::OverrideResult::NoAction: + break; + case FunctionEffectDiff::OverrideResult::Warn: + Diag(New->getLocation(), diag::warn_mismatched_func_effect_override) + << Diff.effectName(); + Diag(Old->getLocation(), diag::note_overridden_virtual_function) + << Old->getReturnTypeSourceRange(); + break; + case FunctionEffectDiff::OverrideResult::Merge: { + NewFX.insert(Diff.Old, Errs); + const auto *NewFT = New->getType()->castAs(); + FunctionProtoType::ExtProtoInfo EPI = NewFT->getExtProtoInfo(); + EPI.FunctionEffects = FunctionEffectsRef(NewFX); + QualType ModQT = Context.getFunctionType(NewFT->getReturnType(), + NewFT->getParamTypes(), EPI); + New->setType(ModQT); + break; + } + } } + if (!Errs.empty()) + diagnoseFunctionEffectMergeConflicts(Errs, New->getLocation(), + Old->getLocation()); } - if (!Errs.empty()) - diagnoseFunctionEffectMergeConflicts(Errs, New->getLocation(), - Old->getLocation()); } CallingConv NewCC = NewFT->getCallConv(), OldCC = OldFT->getCallConv(); diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp index 074062ebbb594f..472e7ae5d1d3f1 100644 --- a/clang/lib/Sema/SemaOverload.cpp +++ b/clang/lib/Sema/SemaOverload.cpp @@ -1863,7 +1863,7 @@ bool Sema::IsFunctionConversion(QualType FromType, QualType ToType, // we need to not alter FromFn, or else even an innocuous cast // like dropping effects will fail. In C++ however we do want to // alter FromFn (because of the way PerformImplicitConversion works). - if (getLangOpts().CPlusPlus) { + if (Context.hasAnyFunctionEffects() && getLangOpts().CPlusPlus) { FromFPT = cast(FromFn); // in case FromFn changed above // Transparently add/drop effects; here we are concerned with