From ba270e09a42fab5364bbdf3f0bee44bf8d5d13c6 Mon Sep 17 00:00:00 2001 From: John Coiner Date: Thu, 5 Oct 2017 18:18:11 -0400 Subject: [PATCH] Add --no-relative-cfuncs and related default optimization, bug1224. Signed-off-by: Wilson Snyder --- Changes | 2 + bin/verilator | 17 +++ src/V3Descope.cpp | 135 ++++++++++++------ src/V3Localize.cpp | 2 +- src/V3Options.cpp | 6 +- src/V3Options.h | 4 +- test_regress/t/t_inst_tree_inl0_pub1.pl | 36 ++++- .../t/t_inst_tree_inl0_pub1_norelcfuncs.pl | 41 ++++++ 8 files changed, 195 insertions(+), 48 deletions(-) create mode 100755 test_regress/t/t_inst_tree_inl0_pub1_norelcfuncs.pl diff --git a/Changes b/Changes index 82c21cea4..1b7bc3b35 100644 --- a/Changes +++ b/Changes @@ -11,6 +11,8 @@ The contributors that suggested a given feature are shown in []. Thanks! *** Add --x-initial option for specifying initial value assignment behavior. +*** Add --no-relative-cfuncs and related default optimization, bug1224. [John Coiner] + **** The internal test_verilated test directory is moved to be part of test_regress. **** Fix over-aggressive inlining, bug1223. [John Coiner] diff --git a/bin/verilator b/bin/verilator index fcd0f87ad..d647c032b 100755 --- a/bin/verilator +++ b/bin/verilator @@ -328,6 +328,7 @@ descriptions in the next sections for more information. --public Debugging; see docs -pvalue+= Overwrite toplevel parameter --relative-includes Resolve includes relative to current file + --no-relative-cfuncs Disallow 'this->' in generated functions --report-unoptflat Extra diagnostics for UNOPTFLAT --savable Enable model save-restore --sc Create SystemC output @@ -880,6 +881,22 @@ change the ultimate model's performance, but may in some cases. Backward compatible alias for "--pins-bv 33". +=item --no-relative-cfuncs + +Disable 'this->' references in generated functions, and instead Verilator +will generate absolute references starting from 'vlTOPp->'. This prevents +V3Combine from merging functions from multiple instances of the same +module, so it can grow the instruction stream. + +This is a work around for old compilers. Don't set this if your C++ +compiler supports __restrict__ properly, as GCC 4.5.x and newer do. For +older compilers, test if this switch gives you better performance or not. + +Compilers which don't honor __restrict__ will suspect that 'this->' +references and 'vlTOPp->' references may alias, and may write slow code +with extra loads and stores to handle the (imaginary) aliasing. Using only +'vlTOPp->' references allows these old compilers to produce tight code. + =item --no-skip-identical Rarely needed. Disables skipping execution of Verilator if all source diff --git a/src/V3Descope.cpp b/src/V3Descope.cpp index 87f17ac53..8871e6da5 100644 --- a/src/V3Descope.cpp +++ b/src/V3Descope.cpp @@ -53,7 +53,8 @@ private: // STATE AstNodeModule* m_modp; // Current module AstScope* m_scopep; // Current scope - bool m_needThis; // Add thisp to function + bool m_modSingleton; // m_modp is only instanced once + bool m_needThis; // Make function non-static FuncMmap m_modFuncs; // Name of public functions added // METHODS @@ -63,41 +64,91 @@ private: return level; } - string descopedName(AstScope* scopep, bool& hierThisr, AstVar* varp=NULL) { + static bool modIsSingleton(AstNodeModule* modp) { + // True iff there's exactly one instance of this module in the design. + int instances = 0; + for (AstNode* stmtp = modp->stmtsp(); stmtp; stmtp=stmtp->nextp()) { + if (stmtp->castScope()) { + if (++instances > 1) { return false; } + } + } + return (instances == 1); + } + + // Construct the best prefix to reference an object in 'scopep' + // from a CFunc in 'm_scopep'. Result may be relative + // ("this->[...]") or absolute ("vlTOPp->[...]"). + // + // Using relative references allows V3Combine'ing + // code across multiple instances of the same module. + // + // Sets 'hierThisr' true if the object is local to this scope + // (and could be made into a function-local later in V3Localize), + // false if the object is in another scope. + string descopedName(const AstScope* scopep, bool& hierThisr, + const AstVar* varp=NULL) { UASSERT(scopep, "Var/Func not scoped\n"); - hierThisr = true; + hierThisr = (scopep == m_scopep); + + // It's possible to disable relative references. This is a concession + // to older compilers (gcc < 4.5.x) that don't understand __restrict__ + // well and emit extra ld/st to guard against pointer aliasing + // when this-> and vlTOPp-> are mixed in the same function. + // + // "vlTOPp" is declared "restrict" so better compilers understand + // that it won't alias with "this". + bool relativeRefOk = v3Global.opt.relativeCFuncs(); + + // Use absolute refs in top-scoped routines, keep them static. + // The DPI callback registration depends on representing top-level + // static routines as plain function pointers. That breaks if those + // become true OO routines. + // + // V3Combine wouldn't likely be able to combine top-level + // routines anyway, so there's no harm in keeping these static. + if (m_modp->isTop()) { + relativeRefOk = false; + } + + // Use absolute refs if this scope is the only instance of the module. + // Saves a bit of overhead on passing the 'this' pointer, and there's no + // need to be nice to V3Combine when we have only a single instance. + // The risk that this prevents combining identical logic from differently- + // named but identical modules seems low. + if (m_modSingleton) { + relativeRefOk = false; + } + if (varp && varp->isFuncLocal()) { + hierThisr = true; return ""; // Relative to function, not in this - } else if (scopep == m_scopep && m_modp->isTop()) { - //return ""; // Reference to scope we're in, no need to HIER-> it - return "vlTOPp->"; - } else if (scopep == m_scopep && !m_modp->isTop() - && 0) { // We no longer thisp-> as still get ambiguation problems - m_needThis = true; - return "thisp->"; // this-> but with restricted aliasing - } else if (scopep->aboveScopep() && scopep->aboveScopep()==m_scopep - && 0 // DISABLED: GCC considers the pointers ambiguous, so goes ld/store crazy - ) { - // Reference to scope of cell directly under this module, can just "cell->" - string name = scopep->name(); - string::size_type pos; - if ((pos = name.rfind(".")) != string::npos) { - name.erase(0,pos+1); - } - hierThisr = false; - return name+"->"; - } else { - // Reference to something else, use global variable - UINFO(8," Descope "<name()<name()<aboveScopep()) { // Top - return "vlTOPp->"; // == "vlSymsp->TOPp->", but GCC would suspect aliases - } else { - return scopep->nameVlSym()+"."; - } - } + } else if (relativeRefOk && scopep == m_scopep) { + m_needThis = true; + return "this->"; + } else if (relativeRefOk && scopep->aboveScopep() + && scopep->aboveScopep()==m_scopep) { + // Reference to scope of cell directly under this module, can just "cell->" + string name = scopep->name(); + string::size_type pos; + if ((pos = name.rfind(".")) != string::npos) { + name.erase(0,pos+1); + } + m_needThis = true; + return name+"->"; + } else { + // Reference to something elsewhere, or relative refences + // are disabled. Use global variable + UINFO(8," Descope "<name()<name()<aboveScopep()) { // Top + // We could also return "vlSymsp->TOPp->" here, but GCC would + // suspect aliases. + return "vlTOPp->"; + } else { + return scopep->nameVlSym()+"."; + } + } } void makePublicFuncWrappers() { @@ -182,6 +233,7 @@ private: virtual void visit(AstNodeModule* nodep) { m_modp = nodep; m_modFuncs.clear(); + m_modSingleton = modIsSingleton(m_modp); nodep->iterateChildren(*this); makePublicFuncWrappers(); m_modp = NULL; @@ -222,11 +274,7 @@ private: nodep->iterateChildren(*this); nodep->user1(true); if (m_needThis) { - nodep->v3fatalSrc("old code"); - // Really we should have more node types for backend optimization of this stuff - string text = v3Global.opt.modPrefix() + "_" + m_modp->name() - +"* thisp = &("+m_scopep->nameVlSym()+");\n"; - nodep->addInitsp(new AstCStmt(nodep->fileline(), text)); + nodep->isStatic(false); } // If it's under a scope, move it up to the top if (m_scopep) { @@ -248,11 +296,12 @@ private: } public: // CONSTRUCTORS - explicit DescopeVisitor(AstNetlist* nodep) { - m_modp = NULL; - m_scopep = NULL; - m_needThis = false; - nodep->accept(*this); + explicit DescopeVisitor(AstNetlist* nodep) + : m_modp(NULL), + m_scopep(NULL), + m_modSingleton(false), + m_needThis(false) { + nodep->accept(*this); } virtual ~DescopeVisitor() {} }; diff --git a/src/V3Localize.cpp b/src/V3Localize.cpp index f0a542d04..d34773c24 100644 --- a/src/V3Localize.cpp +++ b/src/V3Localize.cpp @@ -84,7 +84,7 @@ private: virtual void visit(AstVarRef* nodep) { VarFlags flags (nodep->varp()); if (flags.m_done) { - nodep->hiername(""); // Remove thisp-> + nodep->hiername(""); // Remove this-> nodep->hierThis(true); } } diff --git a/src/V3Options.cpp b/src/V3Options.cpp index dbfa0783d..d91fadc7b 100644 --- a/src/V3Options.cpp +++ b/src/V3Options.cpp @@ -677,8 +677,9 @@ void V3Options::parseOptsList(FileLine* fl, const string& optdir, int argc, char else if ( onoff (sw, "-profile-cfuncs", flag/*ref*/) ) { m_profileCFuncs = flag; } else if ( onoff (sw, "-public", flag/*ref*/) ) { m_public = flag; } else if ( !strncmp(sw, "-pvalue+", strlen("-pvalue+"))) { addParameter(string(sw+strlen("-pvalue+")), false); } - else if ( onoff (sw, "-report-unoptflat", flag/*ref*/) ) { m_reportUnoptflat = flag; } + else if ( onoff (sw, "-relative-cfuncs", flag/*ref*/) ) { m_relativeCFuncs = flag; } else if ( onoff (sw, "-relative-includes", flag/*ref*/) ) { m_relativeIncludes = flag; } + else if ( onoff (sw, "-report-unoptflat", flag/*ref*/) ) { m_reportUnoptflat = flag; } else if ( onoff (sw, "-savable", flag/*ref*/) ) { m_savable = flag; } else if ( !strcmp (sw, "-sc") ) { m_outFormatOk = true; m_systemC = true; } else if ( onoff (sw, "-skip-identical", flag/*ref*/) ) { m_skipIdentical = flag; } @@ -1209,8 +1210,9 @@ V3Options::V3Options() { m_preprocOnly = false; m_preprocNoLine = false; m_public = false; - m_reportUnoptflat = false; + m_relativeCFuncs = true; m_relativeIncludes = false; + m_reportUnoptflat = false; m_savable = false; m_skipIdentical = true; m_stats = false; diff --git a/src/V3Options.h b/src/V3Options.h index 65ca885a0..b26c722eb 100644 --- a/src/V3Options.h +++ b/src/V3Options.h @@ -88,8 +88,9 @@ class V3Options { bool m_pinsUint8; // main switch: --pins-uint8 bool m_profileCFuncs;// main switch: --profile-cfuncs bool m_public; // main switch: --public - bool m_reportUnoptflat; // main switch: --report-unoptflat + bool m_relativeCFuncs; // main switch: --relative-cfuncs bool m_relativeIncludes; // main switch: --relative-includes + bool m_reportUnoptflat; // main switch: --report-unoptflat bool m_savable; // main switch: --savable bool m_systemC; // main switch: --sc: System C instead of simple C++ bool m_skipIdentical;// main switch: --skip-identical @@ -244,6 +245,7 @@ class V3Options { bool lintOnly() const { return m_lintOnly; } bool ignc() const { return m_ignc; } bool inhibitSim() const { return m_inhibitSim; } + bool relativeCFuncs() const { return m_relativeCFuncs; } bool reportUnoptflat() const { return m_reportUnoptflat; } bool vpi() const { return m_vpi; } bool xInitialEdge() const { return m_xInitialEdge; } diff --git a/test_regress/t/t_inst_tree_inl0_pub1.pl b/test_regress/t/t_inst_tree_inl0_pub1.pl index e39952266..9dcd55750 100755 --- a/test_regress/t/t_inst_tree_inl0_pub1.pl +++ b/test_regress/t/t_inst_tree_inl0_pub1.pl @@ -13,8 +13,42 @@ compile ( verilator_flags2 => ['+define+NOUSE_INLINE', '+define+USE_PUBLIC', '--stats'], ); +sub checkRelativeRefs { + my ($mod, $expect_relative) = @_; + my $found_relative = 0; + + my $file = "$Self->{obj_dir}/V$Self->{name}_${mod}.cpp"; + my $text = file_contents($file); + + # Remove "this->__VlSymsp" which is noise + $text =~ s/this->__VlSymsp//g; + if ($text =~ m/this->/) { + $found_relative = 1; + } + + if ($found_relative != $expect_relative) { + $Self->error("$file " . + ($found_relative ? "has" : "does not have") . + " relative variable references."); + } +} + if ($Self->{vlt}) { - file_grep ($Self->{stats}, qr/Optimizations, Combined CFuncs\s+(\d+)/i, 16); + # We expect to combine sequent functions across multiple instances of + # l2, l3, l4, l5. If this number drops, please confirm this has not broken. + file_grep ($Self->{stats}, qr/Optimizations, Combined CFuncs\s+(\d+)/i, 59); + + # Expect absolute refs in CFuncs for t (top module) and l1 (because it + # has only one instance) + checkRelativeRefs("t", 0); + checkRelativeRefs("l1", 0); + + # Others should get relative references + checkRelativeRefs("l2", 1); + checkRelativeRefs("l3", 1); + checkRelativeRefs("l4", 1); + checkRelativeRefs("l5__P1", 1); + checkRelativeRefs("l5__P2", 1); } execute ( diff --git a/test_regress/t/t_inst_tree_inl0_pub1_norelcfuncs.pl b/test_regress/t/t_inst_tree_inl0_pub1_norelcfuncs.pl new file mode 100755 index 000000000..ac45684ea --- /dev/null +++ b/test_regress/t/t_inst_tree_inl0_pub1_norelcfuncs.pl @@ -0,0 +1,41 @@ +#!/usr/bin/perl +if (!$::Driver) { use FindBin; exec("$FindBin::Bin/bootstrap.pl", @ARGV, $0); die; } +# DESCRIPTION: Verilator: Verilog Test driver/expect definition +# +# Copyright 2003 by Wilson Snyder. This program is free software; you can +# redistribute it and/or modify it under the terms of either the GNU +# Lesser General Public License Version 3 or the Perl Artistic License +# Version 2.0. + +top_filename("t/t_inst_tree.v"); + +compile ( + verilator_flags2 => ['+define+NOUSE_INLINE', '+define+USE_PUBLIC', '--stats', '--norelative-cfuncs'], + ); + +if ($Self->{vlt}) { + # Fewer optimizations than t_inst_tree_inl0_pub1 which allows + # relative CFuncs: + file_grep ($Self->{stats}, qr/Optimizations, Combined CFuncs\s+(\d+)/i, 16); + + # Should not find any 'this->' except some 'this->__VlSymsp' + my @files = `ls $Self->{obj_dir}/*.cpp`; + foreach my $file (@files) { + chomp $file; + my $text = file_contents($file); + $text =~ s/this->__VlSymsp//g; + if ($text =~ m/this->/) { + $Self->error("$file has unexpected this-> refs when --norelative-cfuncs"); + } + } +} + +execute ( + check_finished=>1, + expect=> +'\] (%m|.*t\.ps): Clocked +', + ); + +ok(1); +1;