llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang Author: Anonmiraj (AnonMiraj) <details> <summary>Changes</summary> While benchmarking with warnings enabled, I found that `AnalysisBasedWarnings::getPolicyInEffectAt` runs at the end of every function body, performing six location-sensitive `isIgnored()` queries. This overhead comes from #<!-- -->136323 ([compile-time impact](https://llvm-compile-time-tracker.com/compare.php?from=2a9f77f6bd48d757b2d45aadcb6cf76ef4b4ef32&to=71ce9e26aec00e4af27a69ccfab8ca1773ed7018&stat=instructions:u)). Since these six diagnostics only depend on the diagnostic state at the query location and whether it is in a system header or macro, we can cache the computed policy rather than recomputing it for every function. The cache flushes when a `#pragma clang diagnostic` changes severities, and it bypasses active diagnostic suppression mappings. Compile-time results for this pr: https://llvm-compile-time-tracker.com/compare.php?from=49de424f45389cb757c3cc8c50daf38d024e2314&to=a61503b54e9568254885777cf89f5ca1586ec99f&stat=instructions%3Au --- Full diff: https://github.com/llvm/llvm-project/pull/212213.diff 4 Files Affected: - (modified) clang/include/clang/Basic/Diagnostic.h (+11) - (modified) clang/include/clang/Sema/AnalysisBasedWarnings.h (+7) - (modified) clang/lib/Sema/AnalysisBasedWarnings.cpp (+41-5) - (modified) clang/lib/Sema/Sema.cpp (+4) ``````````diff diff --git a/clang/include/clang/Basic/Diagnostic.h b/clang/include/clang/Basic/Diagnostic.h index 826b747f2c751..18cbd34c29ce5 100644 --- a/clang/include/clang/Basic/Diagnostic.h +++ b/clang/include/clang/Basic/Diagnostic.h @@ -583,6 +583,17 @@ class DiagnosticsEngine : public RefCountedBase<DiagnosticsEngine> { DiagSuppressionMapping; public: + /// Returns a cache key representing the diagnostic state at \p Loc. + const void *getDiagStateKeyForLoc(SourceLocation Loc) const { + return GetDiagStateForLoc(Loc); + } + + /// True if an active diagnostic suppression mapping makes severity dependent + /// on the file path. + bool hasDiagSuppressionMapping() const { + return static_cast<bool>(DiagSuppressionMapping); + } + explicit DiagnosticsEngine(IntrusiveRefCntPtr<DiagnosticIDs> Diags, DiagnosticOptions &DiagOpts, DiagnosticConsumer *client = nullptr, diff --git a/clang/include/clang/Sema/AnalysisBasedWarnings.h b/clang/include/clang/Sema/AnalysisBasedWarnings.h index 0ed61e56825be..c79e72bf3037f 100644 --- a/clang/include/clang/Sema/AnalysisBasedWarnings.h +++ b/clang/include/clang/Sema/AnalysisBasedWarnings.h @@ -66,6 +66,10 @@ class AnalysisBasedWarnings { Policy PolicyOverrides; void clearOverrides(); + /// Caches results for getPolicyInEffectAt(). + /// Flushed whenever a diagnostic pragma changes severities. + llvm::DenseMap<const void *, Policy> PolicyCache[4]; + /// \name Statistics /// @{ @@ -129,6 +133,9 @@ class AnalysisBasedWarnings { // will override the policy used to issue warnings. Policy &getPolicyOverrides() { return PolicyOverrides; } + /// Drop cached getPolicyInEffectAt() results (diagnostic state changed). + void clearPolicyCache(); + void PrintStats() const; }; diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index e070d9f1a9b85..f0674f80a825c 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2759,23 +2759,59 @@ sema::AnalysisBasedWarnings::Policy sema::AnalysisBasedWarnings::getPolicyInEffectAt(SourceLocation Loc) { using namespace diag; DiagnosticsEngine &D = S.getDiagnostics(); + + // This runs at the end of every function definition, and the checks below + // resolve Loc against the pragma diagnostic state (and system header/macro + // classification) once per queried diagnostic. Those inputs fully determine + // the result, so cache the policy on them instead (PolicyOverrides are + // transient per-function state and are applied after the cache lookup). + const bool Cacheable = !D.hasDiagSuppressionMapping(); + const void *StateKey = nullptr; + unsigned SysIdx = 0; + if (Cacheable) { + StateKey = D.getDiagStateKeyForLoc(Loc); + if (Loc.isValid() && D.hasSourceManager()) { + const SourceManager &SM = D.getSourceManager(); + SysIdx = (SM.isInSystemHeader(SM.getExpansionLoc(Loc)) ? 2u : 0u) | + (SM.isInSystemMacro(Loc) ? 1u : 0u); + } + auto It = PolicyCache[SysIdx].find(StateKey); + if (It != PolicyCache[SysIdx].end()) { + Policy P = It->second; + P.enableCheckUnreachable |= PolicyOverrides.enableCheckUnreachable; + P.enableThreadSafetyAnalysis |= + PolicyOverrides.enableThreadSafetyAnalysis; + P.enableConsumedAnalysis |= PolicyOverrides.enableConsumedAnalysis; + return P; + } + } + Policy P; // Note: The enabled checks should be kept in sync with the switch in // SemaPPCallbacks::PragmaDiagnostic(). P.enableCheckUnreachable = - PolicyOverrides.enableCheckUnreachable || areAnyEnabled(D, Loc, warn_unreachable, warn_unreachable_break, warn_unreachable_return, warn_unreachable_loop_increment); - P.enableThreadSafetyAnalysis = PolicyOverrides.enableThreadSafetyAnalysis || - areAnyEnabled(D, Loc, warn_double_lock); + P.enableThreadSafetyAnalysis = areAnyEnabled(D, Loc, warn_double_lock); - P.enableConsumedAnalysis = PolicyOverrides.enableConsumedAnalysis || - areAnyEnabled(D, Loc, warn_use_in_invalid_state); + P.enableConsumedAnalysis = areAnyEnabled(D, Loc, warn_use_in_invalid_state); + + if (Cacheable) + PolicyCache[SysIdx][StateKey] = P; + + P.enableCheckUnreachable |= PolicyOverrides.enableCheckUnreachable; + P.enableThreadSafetyAnalysis |= PolicyOverrides.enableThreadSafetyAnalysis; + P.enableConsumedAnalysis |= PolicyOverrides.enableConsumedAnalysis; return P; } +void sema::AnalysisBasedWarnings::clearPolicyCache() { + for (auto &M : PolicyCache) + M.clear(); +} + void sema::AnalysisBasedWarnings::clearOverrides() { PolicyOverrides.enableCheckUnreachable = false; PolicyOverrides.enableConsumedAnalysis = false; diff --git a/clang/lib/Sema/Sema.cpp b/clang/lib/Sema/Sema.cpp index 78fbc9e31842d..322ae410ed6cd 100644 --- a/clang/lib/Sema/Sema.cpp +++ b/clang/lib/Sema/Sema.cpp @@ -227,6 +227,10 @@ class SemaPPCallbacks : public PPCallbacks { } void PragmaDiagnostic(SourceLocation Loc, StringRef Namespace, diag::Severity Mapping, StringRef Str) override { + // The pragma changed diagnostic severities; drop any cached analysis + // warning policies derived from the previous state. + S->AnalysisWarnings.clearPolicyCache(); + // If one of the analysis-based diagnostics was enabled while processing // a function, we want to note it in the analysis-based warnings so they // can be run at the end of the function body even if the analysis warnings `````````` </details> https://github.com/llvm/llvm-project/pull/212213 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
