llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-analysis Author: David Stone (davidstone) <details> <summary>Changes</summary> There is no reason to dynamically allocate `llvm::DenseMap` and try to hide the type. A header we include anyway already includes `DenseMap.h` so we save almost no compilation time. This change improves performance by avoiding the dynamic allocation, and simplifies the code considerably. Now that we just have a regular data member, there is also no need for a manual destructor, and the copy / move operations will do the right thing. In `getBlock`, we have some code that a comment claims is implementing memoization, but in reality it does nothing. The relevant expression is a conditional `(*SM)[X] = B`, but `B` is equal to `SM->find(X)->second`. In `Accumulate`, we have a bunch of code to add things to the map for the initial set-up. However, the original code would either find or default construct an element, and then if the found element is equal to the default constructed element it would set it to `B`. Rather than doing this in two steps, we can simply use `try_emplace` to insert if it's not already present. This change is sound only if the new element we are inserting cannot be equal to the default constructed element, but the element type is a pointer and this entire section of code assumes `B` is not null. --- Full diff: https://github.com/llvm/llvm-project/pull/172528.diff 2 Files Affected: - (modified) clang/include/clang/Analysis/CFGStmtMap.h (+6-6) - (modified) clang/lib/Analysis/CFGStmtMap.cpp (+11-33) ``````````diff diff --git a/clang/include/clang/Analysis/CFGStmtMap.h b/clang/include/clang/Analysis/CFGStmtMap.h index 842059daf7560..2a997df20c5eb 100644 --- a/clang/include/clang/Analysis/CFGStmtMap.h +++ b/clang/include/clang/Analysis/CFGStmtMap.h @@ -15,6 +15,7 @@ #define LLVM_CLANG_ANALYSIS_CFGSTMTMAP_H #include "clang/Analysis/CFG.h" +#include "llvm/ADT/DenseMap.h" namespace clang { @@ -22,16 +23,15 @@ class ParentMap; class Stmt; class CFGStmtMap { + using SMap = llvm::DenseMap<const Stmt *, CFGBlock *>; ParentMap *PM; - void *M; + SMap M; - CFGStmtMap(ParentMap *pm, void *m) : PM(pm), M(m) {} - CFGStmtMap(const CFGStmtMap &) = delete; - CFGStmtMap &operator=(const CFGStmtMap &) = delete; + CFGStmtMap(ParentMap *pm, SMap m) : PM(pm), M(std::move(m)) {} -public: - ~CFGStmtMap(); + static void Accumulate(SMap &SM, CFGBlock *B); +public: /// Returns a new CFGMap for the given CFG. It is the caller's /// responsibility to 'delete' this object when done using it. static CFGStmtMap *Build(CFG* C, ParentMap *PM); diff --git a/clang/lib/Analysis/CFGStmtMap.cpp b/clang/lib/Analysis/CFGStmtMap.cpp index a2b213e01cc14..a8de07c0ce10a 100644 --- a/clang/lib/Analysis/CFGStmtMap.cpp +++ b/clang/lib/Analysis/CFGStmtMap.cpp @@ -11,7 +11,6 @@ // //===----------------------------------------------------------------------===// -#include "llvm/ADT/DenseMap.h" #include "clang/AST/ParentMap.h" #include "clang/Analysis/CFG.h" #include "clang/Analysis/CFGStmtMap.h" @@ -19,49 +18,28 @@ using namespace clang; -typedef llvm::DenseMap<const Stmt*, CFGBlock*> SMap; -static SMap *AsMap(void *m) { return (SMap*) m; } - -CFGStmtMap::~CFGStmtMap() { delete AsMap(M); } - const CFGBlock *CFGStmtMap::getBlock(const Stmt *S) const { - SMap *SM = AsMap(M); const Stmt *X = S; // If 'S' isn't in the map, walk the ParentMap to see if one of its ancestors // is in the map. while (X) { - SMap::iterator I = SM->find(X); - if (I != SM->end()) { - CFGBlock *B = I->second; - // Memoize this lookup. - if (X != S) - (*SM)[X] = B; - return B; + auto I = M.find(X); + if (I != M.end()) { + return I->second; } - X = PM->getParentIgnoreParens(X); } return nullptr; } -static void Accumulate(SMap &SM, CFGBlock *B) { +void CFGStmtMap::Accumulate(SMap &SM, CFGBlock *B) { // First walk the block-level expressions. - for (CFGBlock::iterator I = B->begin(), E = B->end(); I != E; ++I) { - const CFGElement &CE = *I; - std::optional<CFGStmt> CS = CE.getAs<CFGStmt>(); - if (!CS) - continue; - - CFGBlock *&Entry = SM[CS->getStmt()]; - // If 'Entry' is already initialized (e.g., a terminator was already), - // skip. - if (Entry) - continue; - - Entry = B; - + for (const CFGElement &CE : *B) { + if (std::optional<CFGStmt> CS = CE.getAs<CFGStmt>()) { + SM.try_emplace(CS->getStmt(), B); + } } // Look at the label of the block. @@ -79,13 +57,13 @@ CFGStmtMap *CFGStmtMap::Build(CFG *C, ParentMap *PM) { if (!C || !PM) return nullptr; - SMap *SM = new SMap(); + SMap SM; // Walk all blocks, accumulating the block-level expressions, labels, // and terminators. for (CFGBlock *BB : *C) - Accumulate(*SM, BB); + Accumulate(SM, BB); - return new CFGStmtMap(PM, SM); + return new CFGStmtMap(PM, std::move(SM)); } `````````` </details> https://github.com/llvm/llvm-project/pull/172528 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
