[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

2016-12-22 Thread Olivier Goffart via Phabricator via cfe-commits
ogoffart updated this revision to Diff 82325.
ogoffart marked an inline comment as done.

https://reviews.llvm.org/D26465

Files:
  include/clang/Basic/Diagnostic.h
  include/clang/Basic/DiagnosticIDs.h
  lib/Basic/Diagnostic.cpp
  lib/Basic/DiagnosticIDs.cpp
  lib/Serialization/ASTReader.cpp

Index: lib/Serialization/ASTReader.cpp
===
--- lib/Serialization/ASTReader.cpp
+++ lib/Serialization/ASTReader.cpp
@@ -5328,6 +5328,10 @@
 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);
   }
 }
   }
Index: lib/Basic/DiagnosticIDs.cpp
===
--- lib/Basic/DiagnosticIDs.cpp
+++ lib/Basic/DiagnosticIDs.cpp
@@ -396,6 +396,23 @@
   return toLevel(getDiagnosticSeverity(DiagID, Loc, Diag));
 }
 
+DiagnosticMapping &
+DiagnosticIDs::getDiagnosticMapping(unsigned DiagID, SourceLocation Loc,
+const DiagnosticsEngine ) 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  = Diag.DiagStates.front();
+  assert( == Diag.DiagStatePoints.front().State);
+  DiagnosticMapping  = 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.
@@ -406,17 +423,11 @@
 DiagnosticIDs::getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,
  const DiagnosticsEngine ) 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  = State->getOrAddMapping((diag::kind)DiagID);
+  DiagnosticMapping  = getDiagnosticMapping(DiagID, Loc, Diag);
 
   // TODO: Can a null severity really get here?
   if (Mapping.getSeverity() != diag::Severity())
Index: lib/Basic/Diagnostic.cpp
===
--- lib/Basic/Diagnostic.cpp
+++ lib/Basic/Diagnostic.cpp
@@ -201,6 +201,11 @@
   }
   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);
Index: include/clang/Basic/DiagnosticIDs.h
===
--- include/clang/Basic/DiagnosticIDs.h
+++ include/clang/Basic/DiagnosticIDs.h
@@ -84,6 +84,7 @@
   unsigned IsPragma : 1;
   unsigned HasNoWarningAsError : 1;
   unsigned HasNoErrorAsFatal : 1;
+  unsigned MightBeOverriden : 1;
 
 public:
   static DiagnosticMapping Make(diag::Severity Severity, bool IsUser,
@@ -94,6 +95,8 @@
 Result.IsPragma = IsPragma;
 Result.HasNoWarningAsError = 0;
 Result.HasNoErrorAsFatal = 0;
+Result.MightBeOverriden = 0;
+
 return Result;
   }
 
@@ -108,6 +111,11 @@
 
   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.
@@ -261,6 +269,10 @@
   getDiagnosticLevel(unsigned DiagID, SourceLocation Loc,
  const DiagnosticsEngine ) const LLVM_READONLY;
 
+  DiagnosticMapping &
+  getDiagnosticMapping(unsigned DiagID, SourceLocation Loc,
+   const DiagnosticsEngine ) const LLVM_READONLY;
+
   diag::Severity
   getDiagnosticSeverity(unsigned DiagID, SourceLocation Loc,

[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

2016-12-22 Thread Olivier Goffart via Phabricator via cfe-commits
ogoffart marked an inline comment as done.
ogoffart added a comment.

In https://reviews.llvm.org/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)




Comment at: lib/Basic/DiagnosticIDs.cpp:423
+Mapping = >State->getOrAddMapping((diag::kind)DiagID);
+  }
 

arphaman wrote:
> 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.
The problem is that most things are private in DiagnosticsEngine, so i made it 
a privte member of DiagnosticIds (which is a friend)


https://reviews.llvm.org/D26465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

2016-11-29 Thread Alex Lorenz via Phabricator via cfe-commits
arphaman added a comment.

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?




Comment at: lib/Basic/DiagnosticIDs.cpp:423
+Mapping = >State->getOrAddMapping((diag::kind)DiagID);
+  }
 

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.


https://reviews.llvm.org/D26465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

2016-11-29 Thread Olivier Goffart via Phabricator via cfe-commits
ogoffart added a comment.

Ping 2


https://reviews.llvm.org/D26465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


[PATCH] D26465: [Diag] Optimize DiagnosticIDs::getDiagnosticSeverity

2016-11-20 Thread Olivier Goffart via cfe-commits
ogoffart added a comment.

ping?


https://reviews.llvm.org/D26465



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits