Remove the GCNNLocals feature (#5080)

Now that the WasmGC spec has settled on a way of validating non-nullable locals,
we no longer need this experimental feature that allowed nonstandard uses of
non-nullable locals.

Author: tlively

CHANGELOG.md

@@ -20,6 +20,8 @@ Current Trunk
 - C API: Add BinaryenAddFunctionWithHeapType() which is like BinaryenAddFunction
   but takes a heap type. The old function is kept for backwards compatibility
   and as a convenience.
+- The nonstandard, experimental gc-nn-locals feature has been removed now that
+  standard non-nullable locals are supported.
 
 v114
 ----

src/ir/type-updating.cpp

@@ -307,10 +307,6 @@ bool canHandleAsLocal(Type type) {
 }
 
 void handleNonDefaultableLocals(Function* func, Module& wasm) {
-  if (wasm.features.hasGCNNLocals()) {
-    // We have nothing to fix up: all locals are allowed.
-    return;
-  }
   if (!wasm.features.hasReferenceTypes()) {
     // No references, so no non-nullable ones at all.
     return;
@@ -404,9 +400,6 @@ void handleNonDefaultableLocals(Function* func, Module& wasm) {
 
 Type getValidLocalType(Type type, FeatureSet features) {
   assert(type.isConcrete());
-  if (features.hasGCNNLocals()) {
-    return type;
-  }
   if (type.isNonNullable()) {
     return Type(type.getHeapType(), Nullable);
   }
@@ -421,9 +414,6 @@ Type getValidLocalType(Type type, FeatureSet features) {
 }
 
 Expression* fixLocalGet(LocalGet* get, Module& wasm) {
-  if (wasm.features.hasGCNNLocals()) {
-    return get;
-  }
   if (get->type.isNonNullable()) {
     // The get should now return a nullable value, and a ref.as_non_null
     // fixes that up.

src/passes/Flatten.cpp

@@ -33,11 +33,9 @@
 //       )
 //      )
 //
-// The tuple has a non-nullable type, and so it cannot be set to a local. We
-// would need to split up the tuple and reconstruct it later, but that would
-// require allowing tuple operations in more nested places than Flat IR allows
-// today. For now, error on this; eventually changes in the spec regarding
-// null-nullability may make this easier.
+// The tuple has a non-nullable type, and so it cannot currently be set to a
+// local, but in principle there's no reason it couldn't be. For now, error on
+// this.
 
 #include <ir/branch-utils.h>
 #include <ir/effects.h>

src/passes/LocalSubtyping.cpp

@@ -78,27 +78,11 @@ struct LocalSubtyping : public WalkerPass<PostWalker<LocalSubtyping>> {
     // Find which vars can be non-nullable.
     std::unordered_set<Index> cannotBeNonNullable;
 
-    if (getModule()->features.hasGCNNLocals()) {
-      // If the feature is enabled then the only constraint is being able to
-      // read the default value - if it is readable, the local cannot become
-      // non-nullable.
-      for (auto& [get, sets] : localGraph.getSetses) {
-        auto index = get->index;
-        if (func->isVar(index) &&
-            std::any_of(sets.begin(), sets.end(), [&](LocalSet* set) {
-              return set == nullptr;
-            })) {
-          cannotBeNonNullable.insert(index);
-        }
-      }
-    } else {
-      // Without GCNNLocals, validation rules follow the spec rules: all gets
-      // must be dominated structurally by sets, for the local to be non-
-      // nullable.
-      LocalStructuralDominance info(func, *getModule());
-      for (auto index : info.nonDominatingIndices) {
-        cannotBeNonNullable.insert(index);
-      }
+    // All gets must be dominated structurally by sets for the local to be non-
+    // nullable.
+    LocalStructuralDominance info(func, *getModule());
+    for (auto index : info.nonDominatingIndices) {
+      cannotBeNonNullable.insert(index);
     }
 
     auto varBase = func->getVarIndexBase();

src/passes/OptimizeInstructions.cpp

@@ -1155,17 +1155,9 @@ struct OptimizeInstructions
 
   void visitLocalSet(LocalSet* curr) {
     // Interactions between local.set/tee and ref.as_non_null can be optimized
-    // in some cases, by removing or moving the ref.as_non_null operation. In
-    // all cases, we only do this when we do *not* allow non-nullable locals. If
-    // we do allow such locals, then (1) this local might be non-nullable, so we
-    // can't remove or move a ref.as_non_null flowing into a local.set/tee, and
-    // (2) even if the local were nullable, if we change things we might prevent
-    // the LocalSubtyping pass from turning it into a non-nullable local later.
-    // Note that we must also check if this local is nullable regardless, as a
-    // parameter might be non-nullable even if nullable locals are disallowed
-    // (as that just affects vars, and not params).
+    // in some cases, by removing or moving the ref.as_non_null operation.
     if (auto* as = curr->value->dynCast<RefAs>()) {
-      if (as->op == RefAsNonNull && !getModule()->features.hasGCNNLocals() &&
+      if (as->op == RefAsNonNull &&
           getFunction()->getLocalType(curr->index).isNullable()) {
         //   (local.tee (ref.as_non_null ..))
         // =>

src/tools/tool-options.h

@@ -89,7 +89,6 @@ struct ToolOptions : public Options {
       .addFeature(FeatureSet::Multivalue, "multivalue functions")
       .addFeature(FeatureSet::GC, "garbage collection")
       .addFeature(FeatureSet::Memory64, "memory64")
-      .addFeature(FeatureSet::GCNNLocals, "GC non-null locals")
       .addFeature(FeatureSet::RelaxedSIMD, "relaxed SIMD")
       .addFeature(FeatureSet::ExtendedConst, "extended const expressions")
       .addFeature(FeatureSet::Strings, "strings")

src/wasm-features.h

@@ -40,21 +40,15 @@ struct FeatureSet {
     Multivalue = 1 << 9,
     GC = 1 << 10,
     Memory64 = 1 << 11,
-    // TODO: Remove this feature when the wasm spec stabilizes.
-    GCNNLocals = 1 << 12,
-    RelaxedSIMD = 1 << 13,
-    ExtendedConst = 1 << 14,
-    Strings = 1 << 15,
-    MultiMemory = 1 << 16,
+    RelaxedSIMD = 1 << 12,
+    ExtendedConst = 1 << 13,
+    Strings = 1 << 14,
+    MultiMemory = 1 << 15,
     MVP = None,
     // Keep in sync with llvm default features:
     // https://github.com/llvm/llvm-project/blob/c7576cb89d6c95f03968076e902d3adfd1996577/clang/lib/Basic/Targets/WebAssembly.cpp#L150-L153
     Default = SignExt | MutableGlobals,
-    // GCNNLocals are opt-in: merely asking for "All" does not apply them. To
-    // get all possible values use AllPossible. See setAll() below for more
-    // details.
-    All = ((1 << 17) - 1) & ~GCNNLocals,
-    AllPossible = (1 << 17) - 1,
+    All = (1 << 16) - 1,
   };
 
   static std::string toString(Feature f) {
@@ -83,8 +77,6 @@ struct FeatureSet {
         return "gc";
       case Memory64:
         return "memory64";
-      case GCNNLocals:
-        return "gc-nn-locals";
       case RelaxedSIMD:
         return "relaxed-simd";
       case ExtendedConst:
@@ -101,7 +93,7 @@ struct FeatureSet {
   std::string toString() const {
     std::string ret;
     uint32_t x = 1;
-    while (x & Feature::AllPossible) {
+    while (x & Feature::All) {
       if (features & x) {
         if (!ret.empty()) {
           ret += ", ";
@@ -133,12 +125,11 @@ struct FeatureSet {
   bool hasMultivalue() const { return (features & Multivalue) != 0; }
   bool hasGC() const { return (features & GC) != 0; }
   bool hasMemory64() const { return (features & Memory64) != 0; }
-  bool hasGCNNLocals() const { return (features & GCNNLocals) != 0; }
   bool hasRelaxedSIMD() const { return (features & RelaxedSIMD) != 0; }
   bool hasExtendedConst() const { return (features & ExtendedConst) != 0; }
   bool hasStrings() const { return (features & Strings) != 0; }
   bool hasMultiMemory() const { return (features & MultiMemory) != 0; }
-  bool hasAll() const { return (features & AllPossible) != 0; }
+  bool hasAll() const { return (features & All) != 0; }
 
   void set(FeatureSet f, bool v = true) {
     features = v ? (features | f) : (features & ~f);
@@ -155,34 +146,18 @@ struct FeatureSet {
   void setMultivalue(bool v = true) { set(Multivalue, v); }
   void setGC(bool v = true) { set(GC, v); }
   void setMemory64(bool v = true) { set(Memory64, v); }
-  void setGCNNLocals(bool v = true) { set(GCNNLocals, v); }
   void setRelaxedSIMD(bool v = true) { set(RelaxedSIMD, v); }
   void setExtendedConst(bool v = true) { set(ExtendedConst, v); }
   void setStrings(bool v = true) { set(Strings, v); }
   void setMultiMemory(bool v = true) { set(MultiMemory, v); }
   void setMVP() { features = MVP; }
-  void setAll() {
-    // Do not set GCNNLocals, which forces the user to opt in to that feature
-    // explicitly. That is, wasm-opt -all will enable GC but *not* enable
-    // non-nullable locals. To get them, do wasm-opt -all --enable-gc-nn-locals
-    // FIXME: When the wasm spec stabilizes, this feature will go away, as the
-    //        non-nullable locals experiment will either become the standard,
-    //        or it will go away.
-    // Leave the old GCNNLocals value unmodified. This makes things like
-    // --enable-gc-nn-locals -all work (that is, if we enable the feature,
-    // then -all does not disable it; it simply does not enable it by itself).
-    auto oldGCNNLocals = hasGCNNLocals();
-    features = AllPossible;
-    setGCNNLocals(oldGCNNLocals);
-  }
+  void setAll() { features = All; }
 
   void enable(const FeatureSet& other) { features |= other.features; }
-  void disable(const FeatureSet& other) {
-    features = features & ~other.features & AllPossible;
-  }
+  void disable(const FeatureSet& other) { features &= ~other.features; }
 
   template<typename F> void iterFeatures(F f) const {
-    for (uint32_t feature = MVP + 1; feature < AllPossible; feature <<= 1) {
+    for (uint32_t feature = MVP + 1; feature < All; feature <<= 1) {
       if (has(feature)) {
         f(static_cast<Feature>(feature));
       }

src/wasm/wasm-binary.cpp

@@ -2627,9 +2627,7 @@ void WasmBinaryReader::readFunctions() {
       }
     }
 
-    if (!wasm.features.hasGCNNLocals()) {
-      TypeUpdating::handleNonDefaultableLocals(func, wasm);
-    }
+    TypeUpdating::handleNonDefaultableLocals(func, wasm);
 
     std::swap(func->epilogLocation, debugLocation);
     currFunction = nullptr;

src/wasm/wasm-validator.cpp

@@ -3195,45 +3195,13 @@ void FunctionValidator::visitFunction(Function* curr) {
 
   if (getModule()->features.hasGC()) {
     // If we have non-nullable locals, verify that local.get are valid.
-    if (!getModule()->features.hasGCNNLocals()) {
-      // Without the special GCNNLocals feature, we implement the spec rules,
-      // that is, a set allows gets until the end of the block.
-      LocalStructuralDominance info(curr, *getModule());
-      for (auto index : info.nonDominatingIndices) {
-        auto localType = curr->getLocalType(index);
-        for (auto type : localType) {
-          shouldBeTrue(!type.isNonNullable(),
-                       index,
-                       "non-nullable local's sets must dominate gets");
-        }
-      }
-    } else {
-      // With the special GCNNLocals feature, we allow gets anywhere, so long as
-      // we can prove they cannot read the null value. (TODO: remove this once
-      // the spec is stable).
-      //
-      // This is slow, so only do it if we find such locals exist at all.
-      bool hasNNLocals = false;
-      for (const auto& var : curr->vars) {
-        if (!var.isDefaultable()) {
-          hasNNLocals = true;
-          break;
-        }
-      }
-      if (hasNNLocals) {
-        LocalGraph graph(curr);
-        for (auto& [get, sets] : graph.getSetses) {
-          auto index = get->index;
-          // It is always ok to read nullable locals, and it is always ok to
-          // read params even if they are non-nullable.
-          if (curr->getLocalType(index).isDefaultable() ||
-              curr->isParam(index)) {
-            continue;
-          }
-          for (auto* set : sets) {
-            shouldBeTrue(!!set, index, "non-nullable local must not read null");
-          }
-        }
+    LocalStructuralDominance info(curr, *getModule());
+    for (auto index : info.nonDominatingIndices) {
+      auto localType = curr->getLocalType(index);
+      for (auto type : localType) {
+        shouldBeTrue(!type.isNonNullable(),
+                     index,
+                     "non-nullable local's sets must dominate gets");
       }
     }
   }

test/binaryen.js/kitchen-sink.js.txt

@@ -29,11 +29,11 @@ Features.ReferenceTypes: 256
 Features.Multivalue: 512
 Features.GC: 1024
 Features.Memory64: 2048
-Features.RelaxedSIMD: 8192
-Features.ExtendedConst: 16384
-Features.Strings: 32768
-Features.MultiMemory: 65536
-Features.All: 126975
+Features.RelaxedSIMD: 4096
+Features.ExtendedConst: 8192
+Features.Strings: 16384
+Features.MultiMemory: 32768
+Features.All: 65535
 InvalidId: 0
 BlockId: 1
 IfId: 2