From 5b9806ae6d8a0311f941ac453267b3c79033c864 Mon Sep 17 00:00:00 2001 From: Geza Lore Date: Sun, 27 Feb 2022 15:54:04 +0000 Subject: [PATCH] Improve V3Combine - Always use a fast function to replace a slow one if available - Iterate to fixed point (i.e.: if combining made more functions identical, combine those too). This will be more useful in the future. - Use only single, const traversal --- src/V3Ast.h | 1 + src/V3AstNodes.h | 2 +- src/V3Combine.cpp | 320 +++++++++++++++++++++++--------------------- src/V3DupFinder.cpp | 13 +- src/V3DupFinder.h | 19 ++- src/V3Hasher.cpp | 6 + src/V3Hasher.h | 3 + 7 files changed, 204 insertions(+), 160 deletions(-) diff --git a/src/V3Ast.h b/src/V3Ast.h index 50fc3dfaf..5667b5db2 100644 --- a/src/V3Ast.h +++ b/src/V3Ast.h @@ -2877,6 +2877,7 @@ public: virtual bool isPure() const override; virtual bool isOutputter() const override { return !isPure(); } AstCFunc* funcp() const { return m_funcp; } + void funcp(AstCFunc* funcp) { m_funcp = funcp; } void argTypes(const string& str) { m_argTypes = str; } string argTypes() const { return m_argTypes; } // op1p reserved for AstCMethodCall diff --git a/src/V3AstNodes.h b/src/V3AstNodes.h index 545fefae5..4e0b793c7 100644 --- a/src/V3AstNodes.h +++ b/src/V3AstNodes.h @@ -8956,7 +8956,7 @@ public: AstScope* scopep() const { return m_scopep; } void scopep(AstScope* nodep) { m_scopep = nodep; } string rtnTypeVoid() const { return ((m_rtnType == "") ? "void" : m_rtnType); } - bool dontCombine() const { return m_dontCombine || isTrace(); } + bool dontCombine() const { return m_dontCombine || isTrace() || entryPoint(); } void dontCombine(bool flag) { m_dontCombine = flag; } bool dontInline() const { return dontCombine() || slow() || funcPublic(); } bool declPrivate() const { return m_declPrivate; } diff --git a/src/V3Combine.cpp b/src/V3Combine.cpp index b057306ac..eaf5e3708 100644 --- a/src/V3Combine.cpp +++ b/src/V3Combine.cpp @@ -27,193 +27,203 @@ #include "V3DupFinder.h" #include "V3Stats.h" #include "V3Ast.h" +#include "V3AstUserAllocator.h" -#include -#include +#include #include -//###################################################################### - -class CombBaseVisitor VL_NOT_FINAL : public VNVisitor { -protected: - // STATE - - // METHODS - virtual ~CombBaseVisitor() override = default; - VL_DEBUG_FUNC; // Declare debug() -}; - -//###################################################################### -// Combine replacement function - -class CombCallVisitor final : CombBaseVisitor { - // Find all CCALLS of each CFUNC, so that we can later rename them -private: +class CombineVisitor final : VNVisitor { // NODE STATE - std::multimap m_callMmap; // Associative array of {function}{call} - // METHODS -public: - void replaceFunc(AstCFunc* oldfuncp, AstCFunc* newfuncp) { - if (oldfuncp == newfuncp) return; - if (newfuncp) { - UINFO(4, " Replace " << oldfuncp << " -WITH-> " << newfuncp << endl); - } else { - UINFO(4, " Remove " << oldfuncp << endl); - } - // Note: m_callMmap modified in loop, so not using equal_range. - for (auto it = m_callMmap.find(oldfuncp); it != m_callMmap.end(); - it = m_callMmap.find(oldfuncp)) { - AstCCall* const oldp = it->second; - UINFO(4, " Called " << oldp << endl); - UASSERT_OBJ(oldp->funcp() == oldfuncp, oldp, - "Call list broken, points to call w/different func"); - if (newfuncp) { - // Replace call to oldfuncp with call to newfuncp - AstNode* const argsp - = oldp->argsp() ? oldp->argsp()->unlinkFrBackWithNext() : nullptr; - AstCCall* const newp = new AstCCall(oldp->fileline(), newfuncp, argsp); - newp->selfPointer(oldp->selfPointer()); - newp->argTypes(oldp->argTypes()); - addCall(newp); // Fix the table, in case the newfuncp itself gets replaced - oldp->replaceWith(newp); - } else { - // Just deleting empty function - oldp->unlinkFrBack(); - } - VL_DO_DANGLING(pushDeletep(oldp), oldp); - m_callMmap.erase(it); // Fix the table, This call has been replaced - } - } - // METHODS - void addCall(AstCCall* nodep) { m_callMmap.emplace(nodep->funcp(), nodep); } + // AstNodeModule::user1() List of AstCFuncs in this module (via m_cfuncs) + // AstCFunc::user1() List of AstCCalls to this function (via m_callSites) + // AstCFunc::user2() bool: Already replaced (in 'process') + // AstCFunc::user3() bool: Marks functions earlier in iteration order (in 'combinePass') + // *::user4() Used by V3Hasher + const VNUser1InUse m_user1InUse; -private: - // VISITORS - virtual void visit(AstCCall* nodep) override { - if (nodep->funcp()->dontCombine()) return; - addCall(nodep); - } - // LCOV_EXCL_START - virtual void visit(AstAddrOfCFunc* nodep) override { - // We cannot yet handle references via AstAddrOfCFunc, but currently those are - // only used in tracing functions, which are not combined. Blow up in case this changes. - if (nodep->funcp()->dontCombine()) return; - nodep->v3fatalSrc( - "Don't know how to combine functions that are referenced via AstAddrOfCFunc"); - } - // LCOV_EXCL_END - - virtual void visit(AstNode* nodep) override { iterateChildren(nodep); } - -public: - // CONSTRUCTORS - CombCallVisitor() = default; - virtual ~CombCallVisitor() override = default; - void main(AstNetlist* nodep) { iterate(nodep); } -}; - -//###################################################################### -// Combine state, as a visitor of each AstNode - -class CombineVisitor final : CombBaseVisitor { -private: - // NODE STATE - // Entire netlist: - const VNUser3InUse m_user3InUse; // Marks replaced AstCFuncs - // VNUser4InUse part of V3Hasher in V3DupFinder + // TYPES + using funcit_t = std::list::iterator; + struct CFuncs { + std::list m_fast; + std::list m_slow; + }; // STATE + AstUser1Allocator m_cfuncs; // AstCFuncs under module + AstUser1Allocator> m_callSites; // Call sites of the AstCFunc + AstNodeModule* m_modp = nullptr; // Current module + const V3Hasher m_hasher; // For hashing VDouble0 m_cfuncsCombined; // Statistic tracking - CombCallVisitor m_call; // Tracking of function call users - V3DupFinder m_dupFinder; // Duplicate finder for CFuncs in module // METHODS - void walkEmptyFuncs() { - for (const auto& itr : m_dupFinder) { - AstCFunc* const oldfuncp = VN_AS(itr.second, CFunc); - UASSERT_OBJ(oldfuncp, itr.second, "Not a CFunc in hash"); - if (!oldfuncp->emptyBody()) continue; - UASSERT_OBJ(!oldfuncp->dontCombine(), oldfuncp, - "dontCombine function should not be in hash"); + VL_DEBUG_FUNC; // Declare debug() - // Remove calls to empty function - UASSERT_OBJ(!oldfuncp->user3(), oldfuncp, "Should not be processed yet"); - UINFO(5, " Drop empty CFunc " << itr.first << " " << oldfuncp << endl); - oldfuncp->user3SetOnce(); // Mark replaced - m_call.replaceFunc(oldfuncp, nullptr); - oldfuncp->unlinkFrBack(); - VL_DO_DANGLING(pushDeletep(oldfuncp), oldfuncp); + void removeEmptyFunctions(std::list& funcps) { + for (funcit_t it = funcps.begin(), nit; it != funcps.end(); it = nit) { + AstCFunc* const funcp = *it; + nit = it; + ++nit; + + if (funcp->emptyBody()) { + // Delete call sites + for (AstCCall* const callp : m_callSites(funcp)) { + VL_DO_DANGLING(callp->unlinkFrBack()->deleteTree(), callp); + } + m_callSites(funcp).clear(); + // Remove from list + funcps.erase(it); + // Delete function + VL_DO_DANGLING(funcp->unlinkFrBack()->deleteTree(), funcp); + } } } - void walkDupFuncs() { - // Do non-slow first as then favors naming functions based on fast name - for (const bool slow : {false, true}) { - for (auto newIt = m_dupFinder.begin(); newIt != m_dupFinder.end(); ++newIt) { - AstCFunc* const newfuncp = VN_AS(newIt->second, CFunc); - UASSERT_OBJ(newfuncp, newIt->second, "Not a CFunc in hash"); - if (newfuncp->user3()) continue; // Already replaced - if (newfuncp->slow() != slow) continue; - auto oldIt = newIt; - ++oldIt; // Skip over current position - for (; oldIt != m_dupFinder.end(); ++oldIt) { - AstCFunc* const oldfuncp = VN_AS(oldIt->second, CFunc); - UASSERT_OBJ(oldfuncp, oldIt->second, "Not a CFunc in hash"); - UASSERT_OBJ(newfuncp != oldfuncp, newfuncp, - "Same function hashed multiple times"); - if (newIt->first != oldIt->first) break; // Iterate over same hashes only - if (oldfuncp->user3()) continue; // Already replaced - if (!newfuncp->sameTree(oldfuncp)) continue; // Different functions + // One pass of combining. Returns true if did replacement. + bool combinePass(std::list& funcps, V3DupFinder& dupFinder) { + const VNUser3InUse user3InUse; - // Replace calls to oldfuncp with calls to newfuncp - UINFO(5, " Replace CFunc " << newIt->first << " " << newfuncp << endl); - UINFO(5, " with " << oldIt->first << " " << oldfuncp << endl); - ++m_cfuncsCombined; - oldfuncp->user3SetOnce(); // Mark replaced - m_call.replaceFunc(oldfuncp, newfuncp); - oldfuncp->unlinkFrBack(); - // Replacement may promote a slow routine to fast path - if (!oldfuncp->slow()) newfuncp->slow(false); - VL_DO_DANGLING(pushDeletep(oldfuncp), oldfuncp); + bool replaced = false; + + // Replace all identical functions with the first function in the list + for (funcit_t it = funcps.begin(), nit; it != funcps.end(); it = nit) { + AstCFunc* const funcp = *it; + nit = it; + ++nit; + + // Remove functions already replaced in the previous iteration + if (funcp->user2()) { + funcps.erase(it); + VL_DO_DANGLING(funcp->unlinkFrBack()->deleteTree(), funcp); + continue; + } + + while (true) { + auto dit = dupFinder.findDuplicate(funcp); + if (dit == dupFinder.end()) break; + + AstCFunc* oldp = VN_AS(dit->second, CFunc); + AstCFunc* newp = funcp; + UASSERT_OBJ(!oldp->user2(), oldp, "Should have been removed from dupFinder"); + + // Swap them, if the duplicate is earlier in the list of functions. This is + // necessary because replacing a call site in a later function might have made that + // function equivalent to an earlier function, but we want the first equivalent + // function in the list to be the canonical one. + if (oldp->user3()) std::swap(oldp, newp); + + // Something is being replaced + UINFO(9, "Replacing " << oldp << endl); + UINFO(9, " with " << newp << endl); + ++m_cfuncsCombined; + replaced = true; + + // Mark as replaced + oldp->user2(true); + + // Redirect the calls + for (AstCCall* const callp : m_callSites(oldp)) { + // For sanity check only + const V3Hash oldHash = m_hasher(callp); + + // Redirect the call + callp->funcp(newp); + + // When redirecting a call to an equivalent function, we do not need to re-hash + // the caller, because the hash of the two calls must be the same, and hence + // the hash of the caller should not change. + UASSERT_OBJ(oldHash == m_hasher.rehash(callp), callp, "Hash changed"); } + + // Erase the replaced duplicate + UASSERT_OBJ(dupFinder.erase(oldp) == 1, oldp, "Replaced node not in dupFinder"); + + // If we just replaced the function we are iterating (because there was an + // equivalent earlier in the list), then move on, as this is on longer a candidate + if (oldp == funcp) break; + } + + // Mark as function earlier in list of functions. + funcp->user3(true); + } + + return replaced; + } + + void process(AstNetlist* netlistp) { + // First, remove empty functions. We need to do this separately, because removing + // calls can change the hashes of the callers. + for (AstNodeModule* modulep = netlistp->modulesp(); modulep; + modulep = VN_AS(modulep->nextp(), NodeModule)) { + removeEmptyFunctions(m_cfuncs(modulep).m_fast); + removeEmptyFunctions(m_cfuncs(modulep).m_slow); + } + + // Combine functions within each module + for (AstNodeModule* modulep = netlistp->modulesp(); modulep; + modulep = VN_AS(modulep->nextp(), NodeModule)) { + // Put fast functions first, so they are preferred over slow functions + auto funcps = std::move(m_cfuncs(modulep).m_fast); + funcps.splice(funcps.end(), m_cfuncs(modulep).m_slow); + + V3DupFinder dupFinder{m_hasher}; + + // First, hash all functions + for (AstCFunc* const funcp : funcps) dupFinder.insert(funcp); + + // Iterate to fixed point + { + const VNUser2InUse user2InUse; + while (combinePass(funcps, dupFinder)) {} } } } // VISITORS virtual void visit(AstNetlist* nodep) override { - m_call.main(nodep); // Track all call sites of each function - iterateChildren(nodep); + // Gather functions and references + iterateChildrenConst(nodep); + // Combine functions + process(nodep); } virtual void visit(AstNodeModule* nodep) override { - UINFO(4, " MOD " << nodep << endl); - m_dupFinder.clear(); - // Compute hash of all CFuncs in the module - iterateChildren(nodep); - if (debug() >= 9) m_dupFinder.dumpFilePrefixed("combine"); - // Walk the hashes removing empty functions - walkEmptyFuncs(); - // Walk the hashes looking for duplicate functions - walkDupFuncs(); + UASSERT_OBJ(!m_modp, nodep, "Should not nest"); + m_modp = nodep; + iterateChildrenConst(nodep); + m_modp = nullptr; } virtual void visit(AstCFunc* nodep) override { + iterateChildrenConst(nodep); if (nodep->dontCombine()) return; - // Hash the entire function - m_dupFinder.insert(nodep); + auto& coll = nodep->slow() ? m_cfuncs(m_modp).m_slow : m_cfuncs(m_modp).m_fast; + coll.emplace_back(nodep); + } + virtual void visit(AstCCall* nodep) override { + iterateChildrenConst(nodep); + AstCFunc* const funcp = nodep->funcp(); + if (funcp->dontCombine()) return; + m_callSites(funcp).emplace_back(nodep); + } + + virtual void visit(AstAddrOfCFunc* nodep) override { + iterateChildrenConst(nodep); + if (nodep->funcp()->dontCombine()) return; + // LCOV_EXCL_START + // We cannot yet handle references via AstAddrOfCFunc, but currently those are + // only used in tracing functions, which are not combined. Blow up in case this changes. + nodep->v3fatalSrc( + "Don't know how to combine functions that are referenced via AstAddrOfCFunc"); + // LCOV_EXCL_END } //-------------------- - // Default: Just iterate - virtual void visit(AstVar*) override {} // Accelerate - virtual void visit(AstNodeStmt* nodep) override {} // Accelerate - virtual void visit(AstNode* nodep) override { iterateChildren(nodep); } + virtual void visit(AstNode* nodep) override { iterateChildrenConst(nodep); } -public: // CONSTRUCTORS explicit CombineVisitor(AstNetlist* nodep) { iterate(nodep); } - virtual ~CombineVisitor() override { - V3Stats::addStat("Optimizations, Combined CFuncs", m_cfuncsCombined); - } + ~CombineVisitor() { V3Stats::addStat("Optimizations, Combined CFuncs", m_cfuncsCombined); } + +public: + static void apply(AstNetlist* netlistp) { CombineVisitor{netlistp}; } }; //###################################################################### @@ -221,6 +231,6 @@ public: void V3Combine::combineAll(AstNetlist* nodep) { UINFO(2, __FUNCTION__ << ": " << endl); - { CombineVisitor{nodep}; } // Destruct before checking + CombineVisitor::apply(nodep); V3Global::dumpCheckGlobalTree("combine", 0, v3Global.opt.dumpTreeLevel(__FILE__) >= 3); } diff --git a/src/V3DupFinder.cpp b/src/V3DupFinder.cpp index dbbb9985f..809c06670 100644 --- a/src/V3DupFinder.cpp +++ b/src/V3DupFinder.cpp @@ -30,6 +30,17 @@ //###################################################################### // V3DupFinder class functions +V3DupFinder::size_type V3DupFinder::erase(AstNode* nodep) { + const auto& er = equal_range(m_hasher(nodep)); + for (iterator it = er.first; it != er.second; ++it) { + if (nodep == it->second) { + erase(it); + return 1; + } + } + return 0; +} + V3DupFinder::iterator V3DupFinder::findDuplicate(AstNode* nodep, V3DupFinderUserSame* checkp) { const auto& er = equal_range(m_hasher(nodep)); for (iterator it = er.first; it != er.second; ++it) { @@ -37,7 +48,7 @@ V3DupFinder::iterator V3DupFinder::findDuplicate(AstNode* nodep, V3DupFinderUser if (nodep == node2p) continue; // Same node is not a duplicate if (checkp && !checkp->isSame(nodep, node2p)) continue; // User says it is not a duplicate if (!nodep->sameTree(node2p)) continue; // Not the same trees - // Found duplicate! + // Found duplicate return it; } return end(); diff --git a/src/V3DupFinder.h b/src/V3DupFinder.h index bf344f0d2..4cf4b485a 100644 --- a/src/V3DupFinder.h +++ b/src/V3DupFinder.h @@ -28,6 +28,7 @@ #include "V3Hasher.h" #include +#include //============================================================================ @@ -43,12 +44,20 @@ class V3DupFinder final : private std::multimap { using Super = std::multimap; // MEMBERS - const V3Hasher m_hasher; + const V3Hasher* const m_hasherp; // Pointer to owned hasher + const V3Hasher& m_hasher; // Reference to hasher public: // CONSTRUCTORS - V3DupFinder(){}; - ~V3DupFinder() = default; + V3DupFinder() + : m_hasherp{new V3Hasher} + , m_hasher{*m_hasherp} {} + V3DupFinder(const V3Hasher& hasher) + : m_hasherp{nullptr} + , m_hasher{hasher} {} + ~V3DupFinder() { + if (m_hasherp) delete m_hasherp; + } // METHODS VL_DEBUG_FUNC; // Declare debug() @@ -63,10 +72,14 @@ public: using Super::end; using Super::erase; using Super::iterator; + using Super::size_type; // Insert node into data structure iterator insert(AstNode* nodep) { return emplace(m_hasher(nodep), nodep); } + // Erase node from data structure + size_type erase(AstNode* nodep); + // Return duplicate, if one was inserted, with optional user check for sameness iterator findDuplicate(AstNode* nodep, V3DupFinderUserSame* checkp = nullptr); diff --git a/src/V3Hasher.cpp b/src/V3Hasher.cpp index 87bb8119e..56652e647 100644 --- a/src/V3Hasher.cpp +++ b/src/V3Hasher.cpp @@ -516,6 +516,12 @@ V3Hash V3Hasher::operator()(AstNode* nodep) const { return V3Hash(nodep->user4()); } +V3Hash V3Hasher::rehash(AstNode* nodep) const { + nodep->user4(0); + HasherVisitor{nodep}; + return V3Hash(nodep->user4()); +} + V3Hash V3Hasher::uncachedHash(const AstNode* nodep) { const HasherVisitor visitor{nodep, HasherVisitor::Uncached{}}; return visitor.finalHash(); diff --git a/src/V3Hasher.h b/src/V3Hasher.h index fe3cabbd6..57e3176a0 100644 --- a/src/V3Hasher.h +++ b/src/V3Hasher.h @@ -45,6 +45,9 @@ public: // Compute hash of node. This method caches the hash in the node's user4(). V3Hash operator()(AstNode* nodep) const; + // Re-compute hash of this node, discarding cached value, but used cached hash of children. + V3Hash rehash(AstNode* nodep) const; + // Compute hash of node, without caching in user4. static V3Hash uncachedHash(const AstNode* nodep); };