Author: David Stone Date: 2025-12-24T12:08:25-07:00 New Revision: 1ef7348682dfc33e17868c2256c126980dd14a98
URL: https://github.com/llvm/llvm-project/commit/1ef7348682dfc33e17868c2256c126980dd14a98 DIFF: https://github.com/llvm/llvm-project/commit/1ef7348682dfc33e17868c2256c126980dd14a98.diff LOG: [clang][NFC] In `CFGStmtMap`, do not use a `void *` data member, just use the object directly (#172528) 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. Added: Modified: clang/include/clang/Analysis/CFGStmtMap.h clang/lib/Analysis/CFGStmtMap.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/CFGStmtMap.h b/clang/include/clang/Analysis/CFGStmtMap.h index 842059daf7560..c1cefbe45a7b5 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,13 @@ 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(); - /// 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..c376788173d0d 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,73 +18,46 @@ 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) { - // 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; - - } - - // Look at the label of the block. - if (Stmt *Label = B->getLabel()) - SM[Label] = B; - - // Finally, look at the terminator. If the terminator was already added - // because it is a block-level expression in another block, overwrite - // that mapping. - if (Stmt *Term = B->getTerminatorStmt()) - SM[Term] = B; -} - 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); + for (CFGBlock *B : *C) { + // First walk the block-level expressions. + for (const CFGElement &CE : *B) { + if (std::optional<CFGStmt> CS = CE.getAs<CFGStmt>()) + SM.try_emplace(CS->getStmt(), B); + } - return new CFGStmtMap(PM, SM); -} + // Look at the label of the block. + if (Stmt *Label = B->getLabel()) + SM[Label] = B; + + // Finally, look at the terminator. If the terminator was already added + // because it is a block-level expression in another block, overwrite + // that mapping. + if (Stmt *Term = B->getTerminatorStmt()) + SM[Term] = B; + } + return new CFGStmtMap(PM, std::move(SM)); +} _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
