integration into ZAM code generation

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<std::shared_ptr<SideEffectsOp>>{};
     bool is_rec = expr_attrs[a][0]->Tag() == TYPE_RECORD;
 
@@ -1133,8 +1132,7 @@ std::vector<const Attr*> 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<ProfileFunc> 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<SideEffectsOp> 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();

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<SideEffectsOp> GetCallSideEffects(const ScriptFunc* f);
 
     const auto& AttrSideEffects() const { return attr_side_effects; }
@@ -391,13 +389,11 @@ protected:
     std::vector<const Attr*> 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,

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::vector<c
     start_e = _start_e;
     end_e = _end_e;
 
-    for ( auto i : ids )
-        if ( i->IsGlobal() || 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;

src/script_opt/Reduce.h

@@ -366,11 +366,6 @@ protected:
     // renders the CSE unsafe.
     const std::vector<const ID*>& 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;

src/script_opt/ScriptOpt.cc

@@ -231,7 +231,7 @@ static void optimize_func(ScriptFunc* f, std::shared_ptr<ProfileFunc> 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();
 

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<ProfileFunc> pf, ScopePtr scope, StmtPtr body,
+    ZAMCompiler(ScriptFunc* f, ProfileFuncs& pfs, std::shared_ptr<ProfileFunc> pf, ScopePtr scope, StmtPtr body,
                 std::shared_ptr<UseDefs> ud, std::shared_ptr<Reducer> rd);
     ~ZAMCompiler();
 
@@ -503,6 +502,7 @@ private:
     std::vector<const NameExpr*> retvars;
 
     ScriptFunc* func;
+    ProfileFuncs& pfs;
     std::shared_ptr<ProfileFunc> pf;
     ScopePtr scope;
     StmtPtr body;

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<ProfileFunc> _pf, ScopePtr _scope, StmtPtr _body,
-                         std::shared_ptr<UseDefs> _ud, std::shared_ptr<Reducer> _rd) {
+ZAMCompiler::ZAMCompiler(ScriptFunc* f, ProfileFuncs& _pfs, std::shared_ptr<ProfileFunc> _pf, ScopePtr _scope,
+                         StmtPtr _body, std::shared_ptr<UseDefs> _ud, std::shared_ptr<Reducer> _rd)
+    : pfs(_pfs) {
     func = f;
     pf = std::move(_pf);
     scope = std::move(_scope);

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.
+            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);
 
+    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<CallExpr*>(c)};
 
@@ -1066,7 +1078,7 @@ const ZAMStmt ZAMCompiler::ConstructRecord(const NameExpr* n, const Expr* e) {
 
     z.t = e->GetType();
 
-    if ( ! rc->GetType<RecordType>()->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);
 }