diff --git a/src/script_opt/ProfileFunc.cc b/src/script_opt/ProfileFunc.cc index 925b62b767..72da2b495a 100644 --- a/src/script_opt/ProfileFunc.cc +++ b/src/script_opt/ProfileFunc.cc @@ -1050,8 +1050,7 @@ void ProfileFuncs::ComputeSideEffects() { } } -void ProfileFuncs::SetSideEffects(const Attr* a, IDSet& non_local_ids, TypeSet& aggrs, - bool& is_unknown) { +void ProfileFuncs::SetSideEffects(const Attr* a, IDSet& non_local_ids, TypeSet& aggrs, bool& is_unknown) { auto seo_vec = std::vector>{}; bool is_rec = expr_attrs[a][0]->Tag() == TYPE_RECORD; @@ -1133,8 +1132,7 @@ std::vector ProfileFuncs::AssociatedAttrs(const Type* t) { return assoc_attrs; } -bool ProfileFuncs::AssessSideEffects(const ExprPtr& e, IDSet& non_local_ids, TypeSet& aggrs, - bool& is_unknown) { +bool ProfileFuncs::AssessSideEffects(const ExprPtr& e, IDSet& non_local_ids, TypeSet& aggrs, bool& is_unknown) { std::shared_ptr pf; if ( e->Tag() == EXPR_NAME && e->GetType()->Tag() == TYPE_FUNC ) @@ -1146,8 +1144,7 @@ bool ProfileFuncs::AssessSideEffects(const ExprPtr& e, IDSet& non_local_ids, Typ return AssessSideEffects(pf.get(), non_local_ids, aggrs, is_unknown); } -bool ProfileFuncs::AssessSideEffects(const ProfileFunc* pf, IDSet& non_local_ids, - TypeSet& aggrs, bool& is_unknown) { +bool ProfileFuncs::AssessSideEffects(const ProfileFunc* pf, IDSet& non_local_ids, TypeSet& aggrs, bool& is_unknown) { if ( pf->DoesIndirectCalls() ) is_unknown = true; @@ -1271,11 +1268,11 @@ bool ProfileFuncs::IsTableWithDefaultAggr(const Type* t) { return false; } -bool ProfileFuncs::GetSideEffects(SideEffectsOp::AccessType access, const Type* t) const { +bool ProfileFuncs::HasSideEffects(SideEffectsOp::AccessType access, const TypePtr& t) const { IDSet nli; TypeSet aggrs; - if ( GetSideEffects(access, t, nli, aggrs) ) + if ( GetSideEffects(access, t.get(), nli, aggrs) ) return true; return ! nli.empty() || ! aggrs.empty(); @@ -1320,8 +1317,7 @@ std::shared_ptr ProfileFuncs::GetCallSideEffects(const ScriptFunc return seo; } -bool ProfileFuncs::GetCallSideEffects(const NameExpr* n, IDSet& non_local_ids, TypeSet& aggrs, - bool& is_unknown) { +bool ProfileFuncs::GetCallSideEffects(const NameExpr* n, IDSet& non_local_ids, TypeSet& aggrs, bool& is_unknown) { // This occurs when the expression is itself a function name, and // in an attribute context indicates an implicit call. auto fid = n->Id(); diff --git a/src/script_opt/ProfileFunc.h b/src/script_opt/ProfileFunc.h index b551b2d787..eb7c76c166 100644 --- a/src/script_opt/ProfileFunc.h +++ b/src/script_opt/ProfileFunc.h @@ -324,15 +324,13 @@ public: // ### // true = unknown - bool GetSideEffects(SideEffectsOp::AccessType access, const Type* t) const; - bool GetSideEffects(SideEffectsOp::AccessType access, const Type* t, IDSet& non_local_ids, - TypeSet& aggrs) const; + bool HasSideEffects(SideEffectsOp::AccessType access, const TypePtr& t) const; + bool GetSideEffects(SideEffectsOp::AccessType access, const Type* t, IDSet& non_local_ids, TypeSet& aggrs) const; // Returns nil if side effects are not available. That should never be // the case after we've done our initial analysis, but is provided // as a signal so that this method can also be used during that analysis. - bool GetCallSideEffects(const NameExpr* n, IDSet& non_local_ids, TypeSet& aggrs, - bool& is_unknown); + bool GetCallSideEffects(const NameExpr* n, IDSet& non_local_ids, TypeSet& aggrs, bool& is_unknown); std::shared_ptr GetCallSideEffects(const ScriptFunc* f); const auto& AttrSideEffects() const { return attr_side_effects; } @@ -391,13 +389,11 @@ protected: std::vector AssociatedAttrs(const Type* t); // ### False on can't-make-decision-yet - bool AssessSideEffects(const ExprPtr& e, IDSet& non_local_ids, TypeSet& types, - bool& is_unknown); - bool AssessSideEffects(const ProfileFunc* e, IDSet& non_local_ids, TypeSet& types, - bool& is_unknown); + bool AssessSideEffects(const ExprPtr& e, IDSet& non_local_ids, TypeSet& types, bool& is_unknown); + bool AssessSideEffects(const ProfileFunc* e, IDSet& non_local_ids, TypeSet& types, bool& is_unknown); - bool AssessAggrEffects(SideEffectsOp::AccessType access, const Type* t, IDSet& non_local_ids, - TypeSet& aggrs, bool& is_unknown); + bool AssessAggrEffects(SideEffectsOp::AccessType access, const Type* t, IDSet& non_local_ids, TypeSet& aggrs, + bool& is_unknown); // true = is unknown bool AssessSideEffects(const SideEffectsOp* se, SideEffectsOp::AccessType access, const Type* t, diff --git a/src/script_opt/Reduce.cc b/src/script_opt/Reduce.cc index 1503f62edc..a816702d93 100644 --- a/src/script_opt/Reduce.cc +++ b/src/script_opt/Reduce.cc @@ -437,7 +437,7 @@ bool Reducer::ExprValid(const ID* id, const Expr* e1, const Expr* e2) const { auto aggr = e1->GetOp1(); auto aggr_t = aggr->GetType(); - if ( pfs.GetSideEffects(SideEffectsOp::READ, aggr_t.get()) ) + if ( pfs.HasSideEffects(SideEffectsOp::READ, aggr_t) ) has_side_effects = true; else if ( aggr_t->Tag() == TYPE_TABLE && pfs.IsTableWithDefaultAggr(aggr_t.get()) ) @@ -445,7 +445,7 @@ bool Reducer::ExprValid(const ID* id, const Expr* e1, const Expr* e2) const { } else if ( e1->Tag() == EXPR_RECORD_CONSTRUCTOR || e1->Tag() == EXPR_RECORD_COERCE ) - has_side_effects = pfs.GetSideEffects(SideEffectsOp::CONSTRUCTION, e1->GetType().get()); + has_side_effects = pfs.HasSideEffects(SideEffectsOp::CONSTRUCTION, e1->GetType()); e1_se = ExprSideEffects(has_side_effects); } @@ -828,10 +828,6 @@ CSE_ValidityChecker::CSE_ValidityChecker(ProfileFuncs& _pfs, const std::vectorIsGlobal() || IsAggr(i->GetType()) ) - have_sensitive_IDs = true; - // Track whether this is a record assignment, in which case // we're attuned to assignments to the same field for the // same type of record. @@ -1088,8 +1084,7 @@ bool CSE_ValidityChecker::CheckSideEffects(SideEffectsOp::AccessType access, con return CheckSideEffects(non_local_ids, aggrs); } -bool CSE_ValidityChecker::CheckSideEffects(const IDSet& non_local_ids, - const TypeSet& aggrs) const { +bool CSE_ValidityChecker::CheckSideEffects(const IDSet& non_local_ids, const TypeSet& aggrs) const { if ( non_local_ids.empty() && aggrs.empty() ) // This is far and away the most common case. return false; diff --git a/src/script_opt/Reduce.h b/src/script_opt/Reduce.h index 741ca794d5..dc2febe747 100644 --- a/src/script_opt/Reduce.h +++ b/src/script_opt/Reduce.h @@ -366,11 +366,6 @@ protected: // renders the CSE unsafe. const std::vector& ids; - // Whether the list of identifiers includes some that we need to evaluate - // for being affected by side effects from function calls, table - // accesses, etc. - bool have_sensitive_IDs = false; - // Where in the AST to start our analysis. This is the initial // assignment expression. const Expr* start_e; diff --git a/src/script_opt/ScriptOpt.cc b/src/script_opt/ScriptOpt.cc index 849c6cd480..1ca1199528 100644 --- a/src/script_opt/ScriptOpt.cc +++ b/src/script_opt/ScriptOpt.cc @@ -231,7 +231,7 @@ static void optimize_func(ScriptFunc* f, std::shared_ptr pf, Profil f->SetFrameSize(new_frame_size); if ( analysis_options.gen_ZAM_code ) { - ZAMCompiler ZAM(f, pf, scope, new_body, ud, rc); + ZAMCompiler ZAM(f, pfs, pf, scope, new_body, ud, rc); new_body = ZAM.CompileBody(); diff --git a/src/script_opt/ZAM/Compile.h b/src/script_opt/ZAM/Compile.h index 7d3c29bfbd..e25ac57fcc 100644 --- a/src/script_opt/ZAM/Compile.h +++ b/src/script_opt/ZAM/Compile.h @@ -5,6 +5,7 @@ #pragma once #include "zeek/Event.h" +#include "zeek/script_opt/ProfileFunc.h" #include "zeek/script_opt/UseDefs.h" #include "zeek/script_opt/ZAM/ZBody.h" @@ -23,8 +24,6 @@ class Stmt; class SwitchStmt; class CatchReturnStmt; -class ProfileFunc; - using InstLabel = ZInstI*; // Class representing a single compiled statement. (This is different from, @@ -53,7 +52,7 @@ public: class ZAMCompiler { public: - ZAMCompiler(ScriptFunc* f, std::shared_ptr pf, ScopePtr scope, StmtPtr body, + ZAMCompiler(ScriptFunc* f, ProfileFuncs& pfs, std::shared_ptr pf, ScopePtr scope, StmtPtr body, std::shared_ptr ud, std::shared_ptr rd); ~ZAMCompiler(); @@ -503,6 +502,7 @@ private: std::vector retvars; ScriptFunc* func; + ProfileFuncs& pfs; std::shared_ptr pf; ScopePtr scope; StmtPtr body; diff --git a/src/script_opt/ZAM/Driver.cc b/src/script_opt/ZAM/Driver.cc index 390ad15788..be6e165907 100644 --- a/src/script_opt/ZAM/Driver.cc +++ b/src/script_opt/ZAM/Driver.cc @@ -8,14 +8,14 @@ #include "zeek/Reporter.h" #include "zeek/Scope.h" #include "zeek/module_util.h" -#include "zeek/script_opt/ProfileFunc.h" #include "zeek/script_opt/ScriptOpt.h" #include "zeek/script_opt/ZAM/Compile.h" namespace zeek::detail { -ZAMCompiler::ZAMCompiler(ScriptFunc* f, std::shared_ptr _pf, ScopePtr _scope, StmtPtr _body, - std::shared_ptr _ud, std::shared_ptr _rd) { +ZAMCompiler::ZAMCompiler(ScriptFunc* f, ProfileFuncs& _pfs, std::shared_ptr _pf, ScopePtr _scope, + StmtPtr _body, std::shared_ptr _ud, std::shared_ptr _rd) + : pfs(_pfs) { func = f; pf = std::move(_pf); scope = std::move(_scope); diff --git a/src/script_opt/ZAM/Expr.cc b/src/script_opt/ZAM/Expr.cc index 47dc9de05a..bf6d005918 100644 --- a/src/script_opt/ZAM/Expr.cc +++ b/src/script_opt/ZAM/Expr.cc @@ -652,11 +652,10 @@ const ZAMStmt ZAMCompiler::CompileIndex(const NameExpr* n1, int n2_slot, const T z = ZInstI(zop, Frame1Slot(n1, zop), n2_slot, c3); } - // See the discussion in CSE_ValidityChecker::PreExpr - // regarding always needing to treat this as potentially - // modifying globals. - z.aux = new ZInstAux(0); - z.aux->can_change_non_locals = true; + if ( pfs.HasSideEffects(SideEffectsOp::READ, n2t) ) { + z.aux = new ZInstAux(0); + z.aux->can_change_non_locals = true; + } return AddInst(z); } @@ -830,6 +829,9 @@ const ZAMStmt ZAMCompiler::AssignTableElem(const Expr* e) { z.aux = InternalBuildVals(op2); z.t = op3->GetType(); + if ( pfs.HasSideEffects(SideEffectsOp::WRITE, op1->GetType()) ) + z.aux->can_change_non_locals = true; + return AddInst(z); } @@ -981,7 +983,17 @@ const ZAMStmt ZAMCompiler::DoCall(const CallExpr* c, const NameExpr* n) { if ( ! z.aux ) z.aux = new ZInstAux(0); - z.aux->can_change_non_locals = true; + if ( ! indirect ) { + IDSet non_local_ids; + TypeSet aggrs; + bool is_unknown = false; + + auto resolved = pfs.GetCallSideEffects(func, non_local_ids, aggrs, is_unknown); + ASSERT(resolved); + + if ( is_unknown || ! non_local_ids.empty() || ! aggrs.empty() ) + z.aux->can_change_non_locals = true; + } z.call_expr = {NewRef{}, const_cast(c)}; @@ -1066,7 +1078,7 @@ const ZAMStmt ZAMCompiler::ConstructRecord(const NameExpr* n, const Expr* e) { z.t = e->GetType(); - if ( ! rc->GetType()->IdempotentCreation() ) + if ( pfs.HasSideEffects(SideEffectsOp::CONSTRUCTION, z.t) ) z.aux->can_change_non_locals = true; return AddInst(z); @@ -1165,6 +1177,9 @@ const ZAMStmt ZAMCompiler::RecordCoerce(const NameExpr* n, const Expr* e) { // Mark the integer entries in z.aux as not being frame slots as usual. z.aux->slots = nullptr; + if ( pfs.HasSideEffects(SideEffectsOp::CONSTRUCTION, e->GetType()) ) + z.aux->can_change_non_locals = true; + return AddInst(z); }