[PATCH 05/11] Use own class for storing the RejectLogs

[Tobias Grosser]

On 09/06/2014 02:42, Andreas Simbuerger wrote:
> The new RejectLogs class delegates most of the methods to a std::map
> just like before.
> We use the wrapper class to implement a few convenient shortcuts
> (e.g., RejectLogs::hasErrors).
> ---
>   include/polly/ScopDetection.h           | 10 ++++----
>   include/polly/ScopDetectionDiagnostic.h | 42 ++++++++++++++++++++++++++++++++-
>   lib/Analysis/ScopDetection.cpp          | 20 +++++++---------

I would call it RejectLogsContainer and keep the name RejectLogs in the
ScopDetection. This reduces the changes in this patch and the name 
'Logs' is a little to generatic for ScopDetection.

Otherwise LGTM, please commit without waiting for the other patches.

Cheers,
Tobias


>   3 files changed, 54 insertions(+), 18 deletions(-)
>
> diff --git a/include/polly/ScopDetection.h b/include/polly/ScopDetection.h
> index 5bab520..1202eb5 100644
> --- a/include/polly/ScopDetection.h
> +++ b/include/polly/ScopDetection.h
> @@ -144,7 +144,7 @@ class ScopDetection : public FunctionPass {
>     RegionSet ValidRegions;
>
>     // Remember a list of errors for every region.
> -  mutable std::map<const Region *, RejectLog> RejectLogs;
> +  mutable RejectLogs Logs;
>
>     // Remember the invalid functions producted by backends;
>     typedef std::set<const Function *> FunctionSet;
> @@ -319,11 +319,11 @@ public:
>     typedef std::map<const Region *, RejectLog>::const_iterator
>     const_reject_iterator;
>
> -  reject_iterator reject_begin() { return RejectLogs.begin(); }
> -  reject_iterator reject_end() { return RejectLogs.end(); }
> +  reject_iterator reject_begin() { return Logs.begin(); }
> +  reject_iterator reject_end() { return Logs.end(); }
>
> -  const_reject_iterator reject_begin() const { return RejectLogs.begin(); }
> -  const_reject_iterator reject_end() const { return RejectLogs.end(); }
> +  const_reject_iterator reject_begin() const { return Logs.begin(); }
> +  const_reject_iterator reject_end() const { return Logs.end(); }
>     //@}
>
>     /// @brief Mark the function as invalid so we will not extract any scop from
> diff --git a/include/polly/ScopDetectionDiagnostic.h b/include/polly/ScopDetectionDiagnostic.h
> index 8b19159..8c864ff 100644
> --- a/include/polly/ScopDetectionDiagnostic.h
> +++ b/include/polly/ScopDetectionDiagnostic.h
> @@ -158,12 +158,52 @@ public:
>
>     iterator begin() const { return ErrorReports.begin(); }
>     iterator end() const { return ErrorReports.end(); }
> -  size_t size() { return ErrorReports.size(); }
> +  size_t size() const { return ErrorReports.size(); }
>
>     const Region *region() const { return R; }
>     void report(RejectReasonPtr Reject) { ErrorReports.push_back(Reject); }
>   };
>
> +/// @brief Store reject logs
> +class RejectLogs {
> +  std::map<const Region *, RejectLog> Logs;
> +
> +public:
> +  typedef std::map<const Region *, RejectLog>::iterator iterator;
> +  typedef std::map<const Region *, RejectLog>::const_iterator const_iterator;
> +
> +  iterator begin() { return Logs.begin(); }
> +  iterator end() { return Logs.end(); }
> +
> +  const_iterator begin() const { return Logs.begin(); }
> +  const_iterator end() const { return Logs.end(); }
> +
> +  void insert(std::pair<const Region *, RejectLog> New) {
> +    auto Result = Logs.insert(New);
> +    assert(Result.second && "Tried to replace an element in the log!");
> +  }
> +
> +  std::map<const Region *, RejectLog>::mapped_type at(const Region *R) {
> +    return Logs.at(R);
> +  }
> +
> +  void clear() { Logs.clear(); }
> +
> +  size_t count(const Region *R) const { return Logs.count(R); }
> +
> +  size_t size(const Region *R) const {
> +    if (!Logs.count(R))
> +      return 0;
> +    return Logs.at(R).size();
> +  }
> +
> +  bool hasErrors(const Region *R) const {
> +    return (Logs.count(R) && Logs.at(R).size() > 0);
> +  }
> +
> +  bool hasErrors(Region *R) const { return hasErrors((const Region *)R); }
> +};
> +
>   //===----------------------------------------------------------------------===//
>   /// @brief Base class for CFG related reject reasons.
>   ///
> diff --git a/lib/Analysis/ScopDetection.cpp b/lib/Analysis/ScopDetection.cpp
> index fbaf07c..0907dd3 100644
> --- a/lib/Analysis/ScopDetection.cpp
> +++ b/lib/Analysis/ScopDetection.cpp
> @@ -213,12 +213,12 @@ bool ScopDetection::isMaxRegionInScop(const Region &R, bool Verify) const {
>   }
>
>   std::string ScopDetection::regionIsInvalidBecause(const Region *R) const {
> -  if (!RejectLogs.count(R))
> +  if (!Logs.count(R))
>       return "";
>
>     // Get the first error we found. Even in keep-going mode, this is the first
>     // reason that caused the candidate to be rejected.
> -  RejectLog Errors = RejectLogs.at(R);
> +  RejectLog Errors = Logs.at(R);
>     return (*Errors.begin())->getMessage();
>   }
>
> @@ -617,7 +617,7 @@ void ScopDetection::findScops(Region &R) {
>       return;
>
>     bool IsValidRegion = isValidRegion(R);
> -  bool HasErrors = RejectLogs.count(&R) > 0;
> +  bool HasErrors = Logs.hasErrors(&R);
>
>     if (IsValidRegion && !HasErrors) {
>       ++ValidRegion;
> @@ -707,12 +707,8 @@ bool ScopDetection::isValidRegion(Region &R) const {
>     bool RegionIsValid = isValidRegion(Context);
>     bool HasErrors = !RegionIsValid || Context.Log.size() > 0;
>
> -  if (PollyTrackFailures && HasErrors) {
> -    // std::map::insert does not replace.
> -    std::pair<reject_iterator, bool> InsertedValue =
> -        RejectLogs.insert(std::make_pair(&R, Context.Log));
> -    assert(InsertedValue.second && "Two logs generated for the same Region.");
> -  }
> +  if (PollyTrackFailures && HasErrors)
> +    Logs.insert(std::make_pair(&R, Context.Log));
>
>     return RegionIsValid;
>   }
> @@ -807,8 +803,8 @@ bool ScopDetection::runOnFunction(llvm::Function &F) {
>     // This should help users with increasing the size of valid Scops.
>     for (const Region *R : ValidRegions) {
>       const Region *Parent = R->getParent();
> -    if ((Parent != TopRegion) && RejectLogs.count(Parent))
> -      emitRejectionRemarks(F, RejectLogs.at(Parent), LI);
> +    if ((Parent != TopRegion) && Logs.count(Parent))
> +      emitRejectionRemarks(F, Logs.at(Parent), LI);
>     }
>
>     if (ReportLevel >= 1)
> @@ -851,7 +847,7 @@ void ScopDetection::print(raw_ostream &OS, const Module *) const {
>
>   void ScopDetection::releaseMemory() {
>     ValidRegions.clear();
> -  RejectLogs.clear();
> +  Logs.clear();
>
>     // Do not clear the invalid function set.
>   }
>