[NFC] Store function effects on functions (#7302)

This fixes an unsettling bug where, with a long-enough sequence of passes,
we might create a function A, compute it has no effects, remove that
function, create another function with the same but with effects, and think
the old effects apply to it. This is basically a danger of using a map from
function name to effects.

To avoid that, store the effects on function objects themselves.

Author: kripken

src/ir/effects.h

@@ -29,8 +29,7 @@ class EffectAnalyzer {
 public:
   EffectAnalyzer(const PassOptions& passOptions, Module& module)
     : ignoreImplicitTraps(passOptions.ignoreImplicitTraps),
-      trapsNeverHappen(passOptions.trapsNeverHappen),
-      funcEffectsMap(passOptions.funcEffectsMap), module(module),
+      trapsNeverHappen(passOptions.trapsNeverHappen), module(module),
       features(module.features) {}
 
   EffectAnalyzer(const PassOptions& passOptions,
@@ -47,7 +46,6 @@ class EffectAnalyzer {
 
   bool ignoreImplicitTraps;
   bool trapsNeverHappen;
-  std::shared_ptr<FuncEffectsMap> funcEffectsMap;
   Module& module;
   FeatureSet features;
 
@@ -523,12 +521,14 @@ class EffectAnalyzer {
         return;
       }
 
+      // Get the target's effects, if they exist. Note that we must handle the
+      // case of the function not yet existing (we may be executed in the middle
+      // of a pass, which may have built up calls but not the targets of those
+      // calls; in such a case, we do not find the targets and therefore assume
+      // we know nothing about the effects, which is safe).
       const EffectAnalyzer* targetEffects = nullptr;
-      if (parent.funcEffectsMap) {
-        auto iter = parent.funcEffectsMap->find(curr->target);
-        if (iter != parent.funcEffectsMap->end()) {
-          targetEffects = &iter->second;
-        }
+      if (auto* target = parent.module.getFunctionOrNull(curr->target)) {
+        targetEffects = target->effects.get();
       }
 
       if (curr->isReturn) {

src/pass.h

@@ -99,11 +99,6 @@ struct InliningOptions {
   Index partialInliningIfs = 0;
 };
 
-// Forward declaration for FuncEffectsMap.
-class EffectAnalyzer;
-
-using FuncEffectsMap = std::unordered_map<Name, EffectAnalyzer>;
-
 struct PassOptions {
   friend Pass;
 
@@ -237,15 +232,6 @@ struct PassOptions {
   // Passes to skip and not run.
   std::unordered_set<std::string> passesToSkip;
 
-  // Effect info computed for functions. One pass can generate this and then
-  // other passes later can benefit from it. It is up to the sequence of passes
-  // to update or discard this when necessary - in particular, when new effects
-  // are added to a function this must be changed or we may optimize
-  // incorrectly. However, it is extremely rare for a pass to *add* effects;
-  // passes normally only remove effects. Passes that do add effects must set
-  // addsEffects() so the pass runner is aware of them.
-  std::shared_ptr<FuncEffectsMap> funcEffectsMap;
-
   // -Os is our default
   static constexpr const int DEFAULT_OPTIMIZE_LEVEL = 2;
   static constexpr const int DEFAULT_SHRINK_LEVEL = 1;

src/passes/GlobalEffects.cpp

@@ -181,30 +181,25 @@ struct GenerateGlobalEffects : public Pass {
       }
     }
 
-    // Generate the final data structure, starting from a blank slate where
-    // nothing is known.
-    auto& funcEffectsMap = getPassOptions().funcEffectsMap;
-    funcEffectsMap.reset();
-
+    // Generate the final data, starting from a blank slate where nothing is
+    // known.
     for (auto& [func, info] : analysis.map) {
+      func->effects.reset();
       if (!info.effects) {
-        // Add no entry to funcEffectsMap, since nothing is known.
         continue;
       }
 
-      // Only allocate a new funcEffectsMap here when we actually have data for
-      // it (which might make later effect computation slightly faster, to
-      // quickly skip the funcEffectsMap code path).
-      if (!funcEffectsMap) {
-        funcEffectsMap = std::make_shared<FuncEffectsMap>();
-      }
-      funcEffectsMap->emplace(func->name, *info.effects);
+      func->effects = std::make_shared<EffectAnalyzer>(*info.effects);
     }
   }
 };
 
 struct DiscardGlobalEffects : public Pass {
-  void run(Module* module) override { getPassOptions().funcEffectsMap.reset(); }
+  void run(Module* module) override {
+    for (auto& func : module->functions) {
+      func->effects.reset();
+    }
+  }
 };
 
 Pass* createGenerateGlobalEffectsPass() { return new GenerateGlobalEffects(); }

src/passes/pass.cpp

@@ -1044,9 +1044,9 @@ void PassRunner::handleAfterEffects(Pass* pass, Function* func) {
     TypeUpdating::handleNonDefaultableLocals(func, *wasm);
   }
 
-  if (options.funcEffectsMap && pass->addsEffects()) {
+  if (pass->addsEffects()) {
     // Effects were added, so discard any computed effects for this function.
-    options.funcEffectsMap->erase(func->name);
+    func->effects.reset();
   }
 }
 

src/wasm.h

@@ -2137,6 +2137,9 @@ struct BinaryLocations {
   std::unordered_map<Function*, FunctionLocations> functions;
 };
 
+// Forward declaration for FuncEffectsMap.
+class EffectAnalyzer;
+
 class Function : public Importable {
 public:
   HeapType type = HeapType(Signature()); // parameters and return value
@@ -2183,6 +2186,13 @@ class Function : public Importable {
     delimiterLocations;
   BinaryLocations::FunctionLocations funcLocation;
 
+  // The effects for this function, if they have been computed. We use a shared
+  // ptr here to avoid compilation errors with the forward-declared
+  // EffectAnalyzer.
+  //
+  // See addsEffects() in pass.h for more details.
+  std::shared_ptr<EffectAnalyzer> effects;
+
   // Inlining metadata: whether to disallow full and/or partial inlining (for
   // details on what those mean, see Inlining.cpp).
   bool noFullInline = false;