[PATCH] D127338: [Clang][CoverageMapping] Fix compile time explosions by adjusting only appropriated skipped ranges

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe6a76a49356e: [Clang][CoverageMapping] Fix compile time 
explosions by adjusting only… (authored by bruno).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127338/new/

https://reviews.llvm.org/D127338

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h

Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -31,15 +31,29 @@
 class Stmt;
 
 struct SkippedRange {
+  enum Kind {
+PPIfElse, // Preprocessor #if/#else ...
+EmptyLine,
+Comment,
+  };
+
   SourceRange Range;
   // The location of token before the skipped source range.
   SourceLocation PrevTokLoc;
   // The location of token after the skipped source range.
   SourceLocation NextTokLoc;
+  // The nature of this skipped range
+  Kind RangeKind;
+
+  bool isComment() { return RangeKind == Comment; }
+  bool isEmptyLine() { return RangeKind == EmptyLine; }
+  bool isPPIfElse() { return RangeKind == PPIfElse; }
 
-  SkippedRange(SourceRange Range, SourceLocation PrevTokLoc = SourceLocation(),
+  SkippedRange(SourceRange Range, Kind K,
+   SourceLocation PrevTokLoc = SourceLocation(),
SourceLocation NextTokLoc = SourceLocation())
-  : Range(Range), PrevTokLoc(PrevTokLoc), NextTokLoc(NextTokLoc) {}
+  : Range(Range), PrevTokLoc(PrevTokLoc), NextTokLoc(NextTokLoc),
+RangeKind(K) {}
 };
 
 /// Stores additional source code information like skipped ranges which
@@ -62,7 +76,7 @@
 
   std::vector () { return SkippedRanges; }
 
-  void AddSkippedRange(SourceRange Range);
+  void AddSkippedRange(SourceRange Range, SkippedRange::Kind RangeKind);
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -60,26 +60,27 @@
   return CoverageInfo;
 }
 
-void CoverageSourceInfo::AddSkippedRange(SourceRange Range) {
+void CoverageSourceInfo::AddSkippedRange(SourceRange Range,
+ SkippedRange::Kind RangeKind) {
   if (EmptyLineCommentCoverage && !SkippedRanges.empty() &&
   PrevTokLoc == SkippedRanges.back().PrevTokLoc &&
   SourceMgr.isWrittenInSameFile(SkippedRanges.back().Range.getEnd(),
 Range.getBegin()))
 SkippedRanges.back().Range.setEnd(Range.getEnd());
   else
-SkippedRanges.push_back({Range, PrevTokLoc});
+SkippedRanges.push_back({Range, RangeKind, PrevTokLoc});
 }
 
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::PPIfElse);
 }
 
 void CoverageSourceInfo::HandleEmptyline(SourceRange Range) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::EmptyLine);
 }
 
 bool CoverageSourceInfo::HandleComment(Preprocessor , SourceRange Range) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::Comment);
   return false;
 }
 
@@ -335,6 +336,8 @@
   /// This shrinks the skipped range if it spans a line that contains a
   /// non-comment token. If shrinking the skipped range would make it empty,
   /// this returns None.
+  /// Note this function can potentially be expensive because
+  /// getSpellingLineNumber uses getLineNumber, which is expensive.
   Optional adjustSkippedRange(SourceManager ,
   SourceLocation LocStart,
   SourceLocation LocEnd,
@@ -382,8 +385,13 @@
   auto CovFileID = getCoverageFileID(LocStart);
   if (!CovFileID)
 continue;
-  Optional SR =
-  adjustSkippedRange(SM, LocStart, LocEnd, I.PrevTokLoc, I.NextTokLoc);
+  Optional SR;
+  if (I.isComment())
+SR = adjustSkippedRange(SM, LocStart, LocEnd, I.PrevTokLoc,
+I.NextTokLoc);
+  else if (I.isPPIfElse() || I.isEmptyLine())
+SR = {SM, LocStart, LocEnd};
+
   if (!SR.hasValue())
 continue;
   auto Region = CounterMappingRegion::makeSkipped(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127338: [Clang][CoverageMapping] Fix compile time explosions by adjusting only appropriated skipped ranges

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno updated this revision to Diff 435388.
bruno added a comment.

Empty lines handling should also make it skipped, fix the 3 failures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127338/new/

https://reviews.llvm.org/D127338

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h

Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -31,15 +31,29 @@
 class Stmt;
 
 struct SkippedRange {
+  enum Kind {
+PPIfElse, // Preprocessor #if/#else ...
+EmptyLine,
+Comment,
+  };
+
   SourceRange Range;
   // The location of token before the skipped source range.
   SourceLocation PrevTokLoc;
   // The location of token after the skipped source range.
   SourceLocation NextTokLoc;
+  // The nature of this skipped range
+  Kind RangeKind;
+
+  bool isComment() { return RangeKind == Comment; }
+  bool isEmptyLine() { return RangeKind == EmptyLine; }
+  bool isPPIfElse() { return RangeKind == PPIfElse; }
 
-  SkippedRange(SourceRange Range, SourceLocation PrevTokLoc = SourceLocation(),
+  SkippedRange(SourceRange Range, Kind K,
+   SourceLocation PrevTokLoc = SourceLocation(),
SourceLocation NextTokLoc = SourceLocation())
-  : Range(Range), PrevTokLoc(PrevTokLoc), NextTokLoc(NextTokLoc) {}
+  : Range(Range), PrevTokLoc(PrevTokLoc), NextTokLoc(NextTokLoc),
+RangeKind(K) {}
 };
 
 /// Stores additional source code information like skipped ranges which
@@ -62,7 +76,7 @@
 
   std::vector () { return SkippedRanges; }
 
-  void AddSkippedRange(SourceRange Range);
+  void AddSkippedRange(SourceRange Range, SkippedRange::Kind RangeKind);
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -60,26 +60,27 @@
   return CoverageInfo;
 }
 
-void CoverageSourceInfo::AddSkippedRange(SourceRange Range) {
+void CoverageSourceInfo::AddSkippedRange(SourceRange Range,
+ SkippedRange::Kind RangeKind) {
   if (EmptyLineCommentCoverage && !SkippedRanges.empty() &&
   PrevTokLoc == SkippedRanges.back().PrevTokLoc &&
   SourceMgr.isWrittenInSameFile(SkippedRanges.back().Range.getEnd(),
 Range.getBegin()))
 SkippedRanges.back().Range.setEnd(Range.getEnd());
   else
-SkippedRanges.push_back({Range, PrevTokLoc});
+SkippedRanges.push_back({Range, RangeKind, PrevTokLoc});
 }
 
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::PPIfElse);
 }
 
 void CoverageSourceInfo::HandleEmptyline(SourceRange Range) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::EmptyLine);
 }
 
 bool CoverageSourceInfo::HandleComment(Preprocessor , SourceRange Range) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::Comment);
   return false;
 }
 
@@ -335,6 +336,8 @@
   /// This shrinks the skipped range if it spans a line that contains a
   /// non-comment token. If shrinking the skipped range would make it empty,
   /// this returns None.
+  /// Note this function can potentially be expensive because
+  /// getSpellingLineNumber uses getLineNumber, which is expensive.
   Optional adjustSkippedRange(SourceManager ,
   SourceLocation LocStart,
   SourceLocation LocEnd,
@@ -382,8 +385,13 @@
   auto CovFileID = getCoverageFileID(LocStart);
   if (!CovFileID)
 continue;
-  Optional SR =
-  adjustSkippedRange(SM, LocStart, LocEnd, I.PrevTokLoc, I.NextTokLoc);
+  Optional SR;
+  if (I.isComment())
+SR = adjustSkippedRange(SM, LocStart, LocEnd, I.PrevTokLoc,
+I.NextTokLoc);
+  else if (I.isPPIfElse() || I.isEmptyLine())
+SR = {SM, LocStart, LocEnd};
+
   if (!SR.hasValue())
 continue;
   auto Region = CounterMappingRegion::makeSkipped(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D127338: [Clang][CoverageMapping] Fix compile time explosions by adjusting only appropriated skipped ranges

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno added a comment.

> Just noticed that 3 test cases failed. Please fix them before landing.

Sure thing, thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127338/new/

https://reviews.llvm.org/D127338

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

[PATCH] D127338: [Clang][CoverageMapping] Fix compile time explosions by adjusting only appropriated skipped ranges

[Zequan Wu via Phabricator via cfe-commits]

zequanwu added a comment.

Just noticed that 3 test cases failed. Please fix them before landing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127338/new/

https://reviews.llvm.org/D127338

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

[PATCH] D127338: [Clang][CoverageMapping] Fix compile time explosions by adjusting only appropriated skipped ranges

[Zequan Wu via Phabricator via cfe-commits]

zequanwu accepted this revision.
zequanwu added a comment.
This revision is now accepted and ready to land.

LGTM, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127338/new/

https://reviews.llvm.org/D127338

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

[PATCH] D127338: [Clang][CoverageMapping] Fix compile time explosions by adjusting only appropriated skipped ranges

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

bruno created this revision.
bruno added reviewers: vsk, zequanwu.
Herald added subscribers: hoy, modimo, wenlei.
Herald added a project: All.
bruno requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

D83592  added comments to be part of skipped 
regions, and as part of that, it
also shrinks a skipped range if it spans a line that contains a non-comment
token. This is done by `adjustSkippedRange`.

The `adjustSkippedRange` currently runs on skipped regions that are not
comments, causing a 5min regression while building a big C++ files without any
comments.

Fix the compile time introduced in D83592  by 
tagging SkippedRange with kind
information and use that to decide what needs additional processing. There are
no good way to add a testcase for this that I'm aware.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D127338

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/lib/CodeGen/CoverageMappingGen.h

Index: clang/lib/CodeGen/CoverageMappingGen.h
===
--- clang/lib/CodeGen/CoverageMappingGen.h
+++ clang/lib/CodeGen/CoverageMappingGen.h
@@ -31,15 +31,29 @@
 class Stmt;
 
 struct SkippedRange {
+  enum Kind {
+PPIfElse, // Preprocessor #if/#else ...
+EmptyLine,
+Comment,
+  };
+
   SourceRange Range;
   // The location of token before the skipped source range.
   SourceLocation PrevTokLoc;
   // The location of token after the skipped source range.
   SourceLocation NextTokLoc;
+  // The nature of this skipped range
+  Kind RangeKind;
+
+  bool isComment() { return RangeKind == Comment; }
+  bool isEmptyLine() { return RangeKind == EmptyLine; }
+  bool isPPIfElse() { return RangeKind == PPIfElse; }
 
-  SkippedRange(SourceRange Range, SourceLocation PrevTokLoc = SourceLocation(),
+  SkippedRange(SourceRange Range, Kind K,
+   SourceLocation PrevTokLoc = SourceLocation(),
SourceLocation NextTokLoc = SourceLocation())
-  : Range(Range), PrevTokLoc(PrevTokLoc), NextTokLoc(NextTokLoc) {}
+  : Range(Range), PrevTokLoc(PrevTokLoc), NextTokLoc(NextTokLoc),
+RangeKind(K) {}
 };
 
 /// Stores additional source code information like skipped ranges which
@@ -62,7 +76,7 @@
 
   std::vector () { return SkippedRanges; }
 
-  void AddSkippedRange(SourceRange Range);
+  void AddSkippedRange(SourceRange Range, SkippedRange::Kind RangeKind);
 
   void SourceRangeSkipped(SourceRange Range, SourceLocation EndifLoc) override;
 
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -60,26 +60,27 @@
   return CoverageInfo;
 }
 
-void CoverageSourceInfo::AddSkippedRange(SourceRange Range) {
+void CoverageSourceInfo::AddSkippedRange(SourceRange Range,
+ SkippedRange::Kind RangeKind) {
   if (EmptyLineCommentCoverage && !SkippedRanges.empty() &&
   PrevTokLoc == SkippedRanges.back().PrevTokLoc &&
   SourceMgr.isWrittenInSameFile(SkippedRanges.back().Range.getEnd(),
 Range.getBegin()))
 SkippedRanges.back().Range.setEnd(Range.getEnd());
   else
-SkippedRanges.push_back({Range, PrevTokLoc});
+SkippedRanges.push_back({Range, RangeKind, PrevTokLoc});
 }
 
 void CoverageSourceInfo::SourceRangeSkipped(SourceRange Range, SourceLocation) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::PPIfElse);
 }
 
 void CoverageSourceInfo::HandleEmptyline(SourceRange Range) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::EmptyLine);
 }
 
 bool CoverageSourceInfo::HandleComment(Preprocessor , SourceRange Range) {
-  AddSkippedRange(Range);
+  AddSkippedRange(Range, SkippedRange::Comment);
   return false;
 }
 
@@ -335,6 +336,8 @@
   /// This shrinks the skipped range if it spans a line that contains a
   /// non-comment token. If shrinking the skipped range would make it empty,
   /// this returns None.
+  /// Note this function can potentially be expensive because
+  /// getSpellingLineNumber uses getLineNumber, which is expensive.
   Optional adjustSkippedRange(SourceManager ,
   SourceLocation LocStart,
   SourceLocation LocEnd,
@@ -382,9 +385,14 @@
   auto CovFileID = getCoverageFileID(LocStart);
   if (!CovFileID)
 continue;
-  Optional SR =
-  adjustSkippedRange(SM, LocStart, LocEnd, I.PrevTokLoc, I.NextTokLoc);
-  if (!SR.hasValue())
+  Optional SR;
+  if (I.isComment())
+SR = adjustSkippedRange(SM, LocStart, LocEnd, I.PrevTokLoc,
+I.NextTokLoc);
+  else if (I.isPPIfElse())
+SR = {SM, LocStart, LocEnd};
+
+  if