[Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

[ogoffart]

Summary

DiagnosticIDs::getDiagnosticSeverity function turns out to take a lot of time in getDecomposedLoc. It is called quite often from different places. (For example from Sema::CheckTemplateArgument which attempt to emit warn_cxx98_compat_* diagnostics.)
In most cases, the diagnostics are not overridden by pragmas and the mapping from the command line is good enough. This commit adds a bit in the the first mapping which specify if we should look up the real mapping.

This saves more than 5% of the parsing time according to perf.

[ogoffart]

ping?

[ogoffart]

Ping 2

[arphaman]

Is there any way to test this change?

This saves more than 5% of the parsing time according to perf.

That sounds great. What did you test the parsing on? Will this patch get similar improvements for code that compiles without errors and warnings?

lib/Basic/DiagnosticIDs.cpp

423

I think it would be better if you wrap this piece of code in a static function that returns DiagnosticMapping &, as it should allow you to get rid of all these . to -> changes below.

[ogoffart]

In D26465#607860, @arphaman wrote:

What did you test the parsing on? Will this patch get similar improvements for code that compiles without errors and warnings?

It was benchamerked with https://github.com/woboq/woboq_codebrowser generating itself.
The code does not contain warnings. (unless you really consider the warn_cxx98_compat_* to be warnings)

lib/Basic/DiagnosticIDs.cpp

423

The problem is that most things are private in DiagnosticsEngine, so i made it a privte member of DiagnosticIds (which is a friend)

include/clang/Basic/Diagnostic.h

 
     DiagnosticMapping &getOrAddMapping(diag::kind Diag);
 
     const_iterator begin() const { return DiagMap.begin(); }
     const_iterator end() const { return DiagMap.end(); }
   };
 
   /// \brief Keeps and automatically disposes all DiagStates that we create.
-  std::list<DiagState> DiagStates;
+  ///
+  /// The first DiagState represents the state set by the command line. But it
+  /// also contains a cache for the defaults that is lazily populated.
+  mutable std::list<DiagState> DiagStates;
 
   /// \brief Represents a point in source where the diagnostic state was
   /// modified because of a pragma.
   ///
   /// 'Loc' can be null if the point represents the diagnostic state
   /// modifications done through the command-line.
   struct DiagStatePoint {
     DiagState *State;

include/clang/Basic/DiagnosticIDs.h

#undef DIAG
   }
 
 class DiagnosticMapping {
   unsigned Severity : 3;
   unsigned IsUser : 1;
   unsigned IsPragma : 1;
   unsigned HasNoWarningAsError : 1;
   unsigned HasNoErrorAsFatal : 1;
+  unsigned MightBeOverriden : 1;
 
 public:
   static DiagnosticMapping Make(diag::Severity Severity, bool IsUser,
                                 bool IsPragma) {
     DiagnosticMapping Result;
     Result.Severity = (unsigned)Severity;
     Result.IsUser = IsUser;
     Result.IsPragma = IsPragma;
     Result.HasNoWarningAsError = 0;
     Result.HasNoErrorAsFatal = 0;
+    Result.MightBeOverriden = 0;
+
     return Result;
   }
 
   diag::Severity getSeverity() const { return (diag::Severity)Severity; }
   void setSeverity(diag::Severity Value) { Severity = (unsigned)Value; }
 
   bool isUser() const { return IsUser; }
   bool isPragma() const { return IsPragma; }
 
   bool hasNoWarningAsError() const { return HasNoWarningAsError; }
   void setNoWarningAsError(bool Value) { HasNoWarningAsError = Value; }
 
   bool hasNoErrorAsFatal() const { return HasNoErrorAsFatal; }
   void setNoErrorAsFatal(bool Value) { HasNoErrorAsFatal = Value; }
+
+  /// Only makes sense for the base state: specify if the diagnostic might be
+  /// changed for other source location
+  bool mightBeOverriden() const { return MightBeOverriden; }
+  void setMightBeOverriden(bool Value) { MightBeOverriden = Value; }
 };
 
 /// \brief Used for handling and querying diagnostic IDs.
 ///
 /// Can be used and shared by multiple Diagnostics for multiple translation units.
 class DiagnosticIDs : public RefCountedBase<DiagnosticIDs> {
 public:
   /// \brief The level of the diagnostic, after it has been through mapping.
private:
   /// DiagnosticsEngine object.
   ///
   /// \param Loc The source location for which we are interested in finding out
   /// the diagnostic state. Can be null in order to query the latest state.
   DiagnosticIDs::Level
   getDiagnosticLevel(unsigned DiagID, SourceLocation Loc,
                      const DiagnosticsEngine &Diag) const LLVM_READONLY;
 
+  DiagnosticMapping &
+  getDiagnosticMapping(unsigned DiagID, SourceLocation Loc,
+                       const DiagnosticsEngine &Diag) const LLVM_READONLY;
+
   diag::Severity
   getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
                         const DiagnosticsEngine &Diag) const LLVM_READONLY;
 
   /// \brief Used to report a diagnostic that is finally fully formed.
   ///
   /// \returns \c true if the diagnostic was emitted, \c false if it was
   /// suppressed.

lib/Basic/Diagnostic.cpp

   if (Map == diag::Severity::Warning) {
     DiagnosticMapping &Info = GetCurDiagState()->getOrAddMapping(Diag);
     if (Info.getSeverity() == diag::Severity::Error ||
         Info.getSeverity() == diag::Severity::Fatal)
       Map = Info.getSeverity();
   }
   DiagnosticMapping Mapping = makeUserMapping(Map, L);
 
+  if (Loc.isValid()) {
+    // We are potentially overriding a default behavior
+    DiagStates.front().getOrAddMapping(Diag).setMightBeOverriden(true);
+  }
+
   // Common case; setting all the diagnostics of a group in one place.
   if (Loc.isInvalid() || Loc == LastStateChangePos) {
     GetCurDiagState()->setMapping(Diag, Mapping);
     return;
   }
 
   // Another common case; modifying diagnostic state in a source location
   // after the previous one.

lib/Basic/DiagnosticIDs.cpp

     return CustomDiagInfo->getLevel(DiagID);
   }
 
   unsigned DiagClass = getBuiltinDiagClass(DiagID);
   if (DiagClass == CLASS_NOTE) return DiagnosticIDs::Note;
   return toLevel(getDiagnosticSeverity(DiagID, Loc, Diag));
 }
 
+DiagnosticMapping &
+DiagnosticIDs::getDiagnosticMapping(unsigned DiagID, SourceLocation Loc,
+                                    const DiagnosticsEngine &Diag) const {
+  // Calling GetDiagStatePointForLoc might be costly as it needs to create
+  // FullSourceLoc and compare them to the ones in the states. So first query
+  // the base state and check if it might be overridden.
+  DiagnosticsEngine::DiagState &BaseState = Diag.DiagStates.front();
+  assert(&BaseState == Diag.DiagStatePoints.front().State);
+  DiagnosticMapping &Mapping = BaseState.getOrAddMapping((diag::kind)DiagID);
+  if (!Mapping.mightBeOverriden())
+    return Mapping;
+
+  DiagnosticsEngine::DiagStatePointsTy::iterator Pos =
+      Diag.GetDiagStatePointForLoc(Loc);
+  return Pos->State->getOrAddMapping((diag::kind)DiagID);
+}
+
 /// \brief Based on the way the client configured the Diagnostic
 /// object, classify the specified diagnostic ID into a Level, consumable by
 /// the DiagnosticClient.
 ///
 /// \param Loc The source location we are interested in finding out the
 /// diagnostic state. Can be null in order to query the latest state.
 diag::Severity
 DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
                                      const DiagnosticsEngine &Diag) const {
   assert(getBuiltinDiagClass(DiagID) != CLASS_NOTE);
-
   // Specific non-error diagnostics may be mapped to various levels from ignored
   // to error.  Errors can only be mapped to fatal.
   diag::Severity Result = diag::Severity::Fatal;
 
-  DiagnosticsEngine::DiagStatePointsTy::iterator
-    Pos = Diag.GetDiagStatePointForLoc(Loc);
-  DiagnosticsEngine::DiagState *State = Pos->State;
-
-  // Get the mapping information, or compute it lazily.
-  DiagnosticMapping &Mapping = State->getOrAddMapping((diag::kind)DiagID);
+  DiagnosticMapping &Mapping = getDiagnosticMapping(DiagID, Loc, Diag);
 
   // TODO: Can a null severity really get here?
   if (Mapping.getSeverity() != diag::Severity())
     Result = Mapping.getSeverity();
 
   // Upgrade ignored diagnostics if -Weverything is enabled.
   if (Diag.EnableAllWarnings && Result == diag::Severity::Ignored &&
       !Mapping.isUser() && getBuiltinDiagClass(DiagID) != CLASS_REMARK)

lib/Serialization/ASTReader.cpp

         }
         unsigned DiagID = F.PragmaDiagMappings[Idx++];
         if (DiagID == (unsigned)-1) {
           break; // no more diag/map pairs for this location.
         }
         diag::Severity Map = (diag::Severity)F.PragmaDiagMappings[Idx++];
         DiagnosticMapping Mapping = Diag.makeUserMapping(Map, Loc);
         Diag.GetCurDiagState()->setMapping(DiagID, Mapping);
+
+        // We are overriding a default behavior
+        Diag.DiagStates.front().getOrAddMapping(DiagID).setMightBeOverriden(
+            true);
       }
     }
   }
 }
 
 /// \brief Get the correct cursor and offset for loading a type.
 ASTReader::RecordLocation ASTReader::TypeCursorForIndex(unsigned Index) {
   GlobalTypeMapType::iterator I = GlobalTypeMap.find(Index);