[wasm] Documentation and small cleanups

Change-Id: Ia3ef956926b54add138936e3e7d03a0faa457ff9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3913127Reviewed-by: Simon Zünd <szuend@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83422}

src/compiler/wasm-escape-analysis.h

@@ -17,9 +17,8 @@ namespace compiler {
 
 class MachineGraph;
 
-// Eliminate allocated objects which are only assigned to.
-// Current restrictions: Does not work for arrays (until they are also allocated
-// with AllocateRaw). Does not work if the allocated object is passed to a phi.
+// Eliminate allocated objects with no uses other than as store targets.
+// Future work: Also exclude phis and renamings from uses.
 class WasmEscapeAnalysis final : public AdvancedReducer {
  public:
   WasmEscapeAnalysis(Editor* editor, MachineGraph* mcgraph)

src/compiler/wasm-gc-operator-reducer.cc

@@ -117,6 +117,9 @@ wasm::TypeInModule WasmGCOperatorReducer::ObjectTypeFromContext(Node* object,
              : type_from_node;
 }
 
+// If the condition of this node's branch is a type check or a null check,
+// add the additional information about the type-checked node to the path
+// state.
 Reduction WasmGCOperatorReducer::ReduceIf(Node* node, bool condition) {
   DCHECK(node->opcode() == IrOpcode::kIfTrue ||
          node->opcode() == IrOpcode::kIfFalse);
@@ -184,7 +187,7 @@ Reduction WasmGCOperatorReducer::ReduceMerge(Node* node) {
     // Change the current type block list to a longest common prefix of this
     // state list and the other list. (The common prefix should correspond to
     // the state of the common dominator.)
-    // TODO(manoskouk): Consider computing intersections for some types.
+    // TODO(manoskouk): Consider computing unions for some types.
     types.ResetToCommonAncestor(GetState(*input_it));
   }
   return UpdateStates(node, types);

src/compiler/wasm-gc-operator-reducer.h

@@ -35,9 +35,11 @@ struct NodeWithType {
   wasm::TypeInModule type;
 };
 
-// This class optimizes away wasm-gc nodes based on the types of their
-// arguments. Although types have been assigned to nodes already, this class
-// also tracks additional type information along control paths.
+// This class optimizes away wasm-gc type checks and casts. Two types of
+// information are used:
+// - Types already marked on graph nodes.
+// - Path-dependent type information that is inferred when a type check is used
+//   as a branch condition.
 class WasmGCOperatorReducer final
     : public AdvancedReducerWithControlPathState<NodeWithType,
                                                  kMultipleInstances> {
@@ -61,6 +63,8 @@ class WasmGCOperatorReducer final
   Reduction ReduceStart(Node* node);
 
   Node* SetType(Node* node, wasm::ValueType type);
+  // Returns the intersection of the type marked on {object} and the type
+  // information about object tracked on {control}'s control path (if present).
   wasm::TypeInModule ObjectTypeFromContext(Node* object, Node* control);
   Reduction UpdateNodeAndAliasesTypes(Node* state_owner,
                                       ControlPathTypes parent_state, Node* node,

src/compiler/wasm-inlining.h

@@ -55,7 +55,12 @@ class WasmInliner final : public AdvancedReducer {
 
   const char* reducer_name() const override { return "WasmInliner"; }
 
+  // Registers (tail) calls to possibly be inlined, prioritized by inlining
+  // heuristics provided by {LexicographicOrdering}.
+  // Only locally defined functions are inlinable, and a limited number of
+  // inlinings of a specific function is allowed.
   Reduction Reduce(Node* node) final;
+  // Inlines calls registered by {Reduce}, until an inlining budget is exceeded.
   void Finalize() final;
 
   static bool graph_size_allows_inlining(size_t graph_size) {

src/compiler/wasm-typer.h

@@ -18,6 +18,11 @@ namespace compiler {
 
 class MachineGraph;
 
+// Recomputes wasm-gc types along the graph to assign the narrowest possible
+// type to each node.
+// Specifically, struct field accesses, array element accesses, phis, type
+// casts, and type guards are retyped.
+// Types in loops are computed to a fixed point.
 class WasmTyper final : public AdvancedReducer {
  public:
   WasmTyper(Editor* editor, MachineGraph* mcgraph, uint32_t function_index);

src/debug/debug-wasm-objects.cc

@@ -964,13 +964,7 @@ Handle<WasmValueObject> WasmValueObject::New(
       }
       break;
     }
-    case wasm::kRtt: {
-      // TODO(7748): Expose RTTs to DevTools.
-      t = isolate->factory()->InternalizeString(base::StaticCharVector("rtt"));
-      v = isolate->factory()->InternalizeString(
-          base::StaticCharVector("(unimplemented)"));
-      break;
-    }
+    case wasm::kRtt:
     case wasm::kVoid:
     case wasm::kBottom:
       UNREACHABLE();

src/diagnostics/objects-printer.cc

@@ -1880,7 +1880,6 @@ void AsmWasmData::AsmWasmDataPrint(std::ostream& os) {
 void WasmTypeInfo::WasmTypeInfoPrint(std::ostream& os) {
   PrintHeader(os, "WasmTypeInfo");
   os << "\n - type address: " << reinterpret_cast<void*>(native_type());
-  // TODO(manoskouk): Print supertype info.
   os << "\n - supertypes: ";
   for (int i = 0; i < supertypes_length(); i++) {
     os << "\n  - " << Brief(supertypes(i));

src/wasm/c-api.cc

@@ -1562,10 +1562,10 @@ void PushArgs(const i::wasm::FunctionSig* sig, const Val args[],
         // TODO(7748): Make sure this works for all heap types.
         packer->Push(WasmRefToV8(store->i_isolate(), args[i].ref())->ptr());
         break;
-      case i::wasm::kRtt:
       case i::wasm::kS128:
         // TODO(7748): Implement.
         UNIMPLEMENTED();
+      case i::wasm::kRtt:
       case i::wasm::kI8:
       case i::wasm::kI16:
       case i::wasm::kVoid:
@@ -1601,10 +1601,10 @@ void PopArgs(const i::wasm::FunctionSig* sig, Val results[],
         results[i] = Val(V8RefValueToWasm(store, obj));
         break;
       }
-      case i::wasm::kRtt:
       case i::wasm::kS128:
         // TODO(7748): Implement.
         UNIMPLEMENTED();
+      case i::wasm::kRtt:
       case i::wasm::kI8:
       case i::wasm::kI16:
       case i::wasm::kVoid:
@@ -1869,10 +1869,10 @@ auto Global::get() const -> Val {
       }
       return Val(V8RefValueToWasm(store, result));
     }
-    case i::wasm::kRtt:
     case i::wasm::kS128:
       // TODO(7748): Implement these.
       UNIMPLEMENTED();
+    case i::wasm::kRtt:
     case i::wasm::kI8:
     case i::wasm::kI16:
     case i::wasm::kVoid:

src/wasm/canonical-types.cc

@@ -83,8 +83,6 @@ uint32_t TypeCanonicalizer::AddRecursiveGroup(const FunctionSig* sig) {
   return canonical_index;
 }
 
-// An index in a type gets mapped to a relative index if it is inside the new
-// canonical group, or the canonical representative if it is not.
 ValueType TypeCanonicalizer::CanonicalizeValueType(
     const WasmModule* module, ValueType type,
     uint32_t recursive_group_start) const {
@@ -116,8 +114,6 @@ bool TypeCanonicalizer::IsCanonicalSubtype(uint32_t sub_index,
   return false;
 }
 
-// Map all type indices (including supertype) inside {type} to indices
-// relative to {recursive_group_start}.
 TypeCanonicalizer::CanonicalType TypeCanonicalizer::CanonicalizeTypeDef(
     const WasmModule* module, TypeDefinition type,
     uint32_t recursive_group_start) {

src/wasm/canonical-types.h

@@ -21,8 +21,8 @@ namespace wasm {
 // types.
 // A recursive group is a subsequence of types explicitly marked in the type
 // section of a wasm module. Identical recursive groups have to be canonicalized
-// to a single canonical group and are considered identical. Respective
-// types in two identical groups are considered identical for all purposes.
+// to a single canonical group. Respective types in two identical groups are
+// considered identical for all purposes.
 // Two groups are considered identical if they have the same shape, and all
 // type indices referenced in the same position in both groups reference:
 // - identical types, if those do not belong to the rec. group,
@@ -105,9 +105,15 @@ class TypeCanonicalizer {
 
   int FindCanonicalGroup(CanonicalGroup&) const;
 
+  // Canonicalize all types present in {type} (including supertype) according to
+  // {CanonicalizeValueType}.
   CanonicalType CanonicalizeTypeDef(const WasmModule* module,
                                     TypeDefinition type,
                                     uint32_t recursive_group_start);
+
+  // An indexed type gets mapped to a {ValueType::CanonicalWithRelativeIndex}
+  // if its index points inside the new canonical group; otherwise, the index
+  // gets mapped to its canonical representative.
   ValueType CanonicalizeValueType(const WasmModule* module, ValueType type,
                                   uint32_t recursive_group_start) const;
 

src/wasm/constant-expression-interface.h

@@ -21,12 +21,13 @@ class JSArrayBuffer;
 
 namespace wasm {
 
-// An interface for WasmFullDecoder used to decode constant expressions. This
-// interface has two modes: only validation (when {isolate_ == nullptr}), which
-// is used in module-decoder, and code-generation (when {isolate_ != nullptr}),
-// which is used in module-instantiate. We merge two distinct functionalities
-// in one class to reduce the number of WasmFullDecoder instantiations, and thus
-// V8 binary code size.
+// An interface for WasmFullDecoder used to decode constant expressions.
+// This interface has two modes: only validation (when {isolate_ == nullptr}),
+// and code-generation (when {isolate_ != nullptr}). We merge two distinct
+// functionalities in one class to reduce the number of WasmFullDecoder
+// instantiations, and thus V8 binary code size.
+// In code-generation mode, the result can be retrieved with {computed_value()}
+// if {!has_error()}, or with {error()} otherwise.
 class V8_EXPORT_PRIVATE ConstantExpressionInterface {
  public:
   static constexpr Decoder::ValidateFlag validate = Decoder::kFullValidation;

src/wasm/function-body-decoder-impl.h

@@ -924,8 +924,7 @@ struct ControlBase : public PcForErrors<validate> {
   F(StartFunctionBody, Control* block) \
   F(FinishFunction)                    \
   F(OnFirstError)                      \
-  F(NextInstruction, WasmOpcode)       \
-  F(Forward, const Value& from, Value* to)
+  F(NextInstruction, WasmOpcode)
 
 #define INTERFACE_CONSTANT_FUNCTIONS(F) /*       force 80 columns           */ \
   F(I32Const, Value* result, int32_t value)                                    \
@@ -979,6 +978,7 @@ struct ControlBase : public PcForErrors<validate> {
     const IndexImmediate<validate>& imm)                                       \
   F(Trap, TrapReason reason)                                                   \
   F(NopForTestingUnsupportedInLiftoff)                                         \
+  F(Forward, const Value& from, Value* to)                                     \
   F(Select, const Value& cond, const Value& fval, const Value& tval,           \
     Value* result)                                                             \
   F(BrOrRet, uint32_t depth, uint32_t drop_values)                             \

src/wasm/function-compiler.h

@@ -44,7 +44,7 @@ struct WasmCompilationResult {
 
   bool succeeded() const { return code_desc.buffer != nullptr; }
   bool failed() const { return !succeeded(); }
-  operator bool() const { return succeeded(); }
+  explicit operator bool() const { return succeeded(); }
 
   CodeDesc code_desc;
   std::unique_ptr<AssemblerBuffer> instr_buffer;

src/wasm/graph-builder-interface.cc

@@ -1685,7 +1685,6 @@ class WasmGraphBuildingInterface {
 
     TFNode* if_success = nullptr;
     TFNode* if_exception = nullptr;
-    // TODO(manoskouk): Can we assign a wasm type to the exception value?
     if (!builder_->ThrowsException(node, &if_success, &if_exception)) {
       return node;
     }
@@ -2073,7 +2072,6 @@ class WasmGraphBuildingInterface {
         }
       }
       if (exception_value != nullptr) {
-        // TODO(manoskouk): Can we assign a wasm type to the exception value?
         *exception_value = builder_->LoopExitValue(
             *exception_value, MachineRepresentation::kWord32);
       }

src/wasm/module-decoder-impl.h

@@ -1345,7 +1345,7 @@ class ModuleDecoderTemplate : public Decoder {
       int64_t last_func_idx = -1;
       for (uint32_t i = 0; i < func_count; i++) {
         uint32_t func_idx = inner.consume_u32v("function index");
-        if (int64_t(func_idx) <= last_func_idx) {
+        if (int64_t{func_idx} <= last_func_idx) {
           inner.errorf("Invalid function index: %d", func_idx);
           break;
         }
@@ -1365,7 +1365,7 @@ class ModuleDecoderTemplate : public Decoder {
           for (uint32_t k = 0; k < mark_size; k++) {
             trace_mark_id |= inner.consume_u8("trace mark id") << k * 8;
           }
-          if (int64_t(func_off) <= last_func_off) {
+          if (int64_t{func_off} <= last_func_off) {
             inner.errorf("Invalid branch offset: %d", func_off);
             break;
           }
@@ -1504,7 +1504,7 @@ class ModuleDecoderTemplate : public Decoder {
       int64_t last_func_idx = -1;
       for (uint32_t i = 0; i < func_count; i++) {
         uint32_t func_idx = inner.consume_u32v("function index");
-        if (int64_t(func_idx) <= last_func_idx) {
+        if (int64_t{func_idx} <= last_func_idx) {
           inner.errorf("Invalid function index: %d", func_idx);
           break;
         }
@@ -1517,7 +1517,7 @@ class ModuleDecoderTemplate : public Decoder {
         int64_t last_br_off = -1;
         for (uint32_t j = 0; j < num_hints; ++j) {
           uint32_t br_off = inner.consume_u32v("branch instruction offset");
-          if (int64_t(br_off) <= last_br_off) {
+          if (int64_t{br_off} <= last_br_off) {
             inner.errorf("Invalid branch offset: %d", br_off);
             break;
           }

src/wasm/wasm-init-expr.h

@@ -24,6 +24,7 @@ class WasmFeatures;
 // Representation of an constant expression. Unlike {ConstantExpression}, this
 // does not use {WireBytesRef}, i.e., it does not depend on a wasm module's
 // bytecode representation.
+// TODO(manoskouk): Add missing kinds of expressions.
 class WasmInitExpr : public ZoneObject {
  public:
   enum Operator {

src/wasm/wasm-js.cc

@@ -1565,8 +1565,6 @@ void WebAssemblyGlobal(const v8::FunctionCallbackInfo<v8::Value>& args) {
       break;
     }
     case i::wasm::kRtt:
-      // TODO(7748): Implement.
-      UNIMPLEMENTED();
     case i::wasm::kI8:
     case i::wasm::kI16:
     case i::wasm::kVoid:
@@ -2725,7 +2723,6 @@ void WebAssemblyGlobalGetValueCommon(
       break;
     }
     case i::wasm::kRtt:
-      UNIMPLEMENTED();  // TODO(7748): Implement.
     case i::wasm::kI8:
     case i::wasm::kI16:
     case i::wasm::kBottom:

src/wasm/wasm-objects.cc

@@ -1554,8 +1554,6 @@ wasm::WasmValue WasmStruct::GetFieldValue(uint32_t index) {
       return wasm::WasmValue(ref, field_type);
     }
     case wasm::kRtt:
-      // TODO(7748): Expose RTTs to DevTools.
-      UNIMPLEMENTED();
     case wasm::kVoid:
     case wasm::kBottom:
       UNREACHABLE();
@@ -1583,8 +1581,6 @@ wasm::WasmValue WasmArray::GetElement(uint32_t index) {
       return wasm::WasmValue(ref, element_type);
     }
     case wasm::kRtt:
-      // TODO(7748): Expose RTTs to DevTools.
-      UNIMPLEMENTED();
     case wasm::kVoid:
     case wasm::kBottom:
       UNREACHABLE();

test/common/wasm/wasm-interpreter.cc

@@ -3668,8 +3668,7 @@ class WasmInterpreterInternals {
             FOREACH_WASMVALUE_CTYPES(CASE_TYPE)
 #undef CASE_TYPE
             case kRef:
-            case kRefNull:
-            case kRtt: {
+            case kRefNull: {
               // TODO(7748): Type checks or DCHECKs for ref types?
               HandleScope handle_scope(isolate_);  // Avoid leaking handles.
               Handle<FixedArray> global_buffer;    // The buffer of the global.
@@ -3681,6 +3680,7 @@ class WasmInterpreterInternals {
               global_buffer->set(global_index, *ref);
               break;
             }
+            case kRtt:
             case kI8:
             case kI16:
             case kVoid:

test/fuzzer/wasm-compile.cc

@@ -842,7 +842,7 @@ class WasmGenerator {
     Var local = GetRandomLocal(data);
     // TODO(manoskouk): Ideally we would check for subtyping here over type
     // equality, but we don't have a module.
-    // TODO(7748): Remove this condition if non-nullable locals are allowed.
+    // TODO(7748): Allow initialized non-nullable locals.
     if (nullable == kNullable && local.is_valid() &&
         local.type.is_object_reference() && type == local.type.heap_type()) {
       builder_->EmitWithU32V(kExprLocalGet, local.index);
@@ -2522,8 +2522,8 @@ class WasmCompileFuzzer : public WasmExecutionFuzzer {
       // performed by adding a function by {FunctionSig}, because we emit
       // everything in one recursive group which blocks signature
       // canonicalization.
-      // TODO(7748): Relax this when we implement type canonicalization and
-      // proper recursive-group support.
+      // TODO(7748): Relax this when we implement proper recursive-group
+      // support.
       functions.push_back(liftoff_as_reference
                               ? builder.AddFunction(function_signatures[i])
                               : builder.AddFunction(sig));

test/mjsunit/wasm/reference-globals.js

@@ -380,3 +380,18 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js");
   assertThrows(() => builder.instantiate(), WebAssembly.CompileError,
                /i31.new\[0\] expected type i32, found i64.const of type i64/);
 })();
+
+(function TestConstantExprFuncIndexOutOfBounds() {
+  print(arguments.callee.name);
+  let builder = new WasmModuleBuilder();
+  let struct_index = builder.addStruct([makeField(kWasmFuncRef, true)]);
+  let func = builder.addFunction("element", kSig_i_i)
+    .addBody([kExprLocalGet, 0])
+    .exportFunc()
+
+  builder.addGlobal(wasmRefType(struct_index), false,
+                    [kExprRefFunc, func.index + 1, kExprStructNew, struct_index]);
+
+  assertThrows(() => builder.instantiate(), WebAssembly.CompileError,
+               /function index #1 is out of bounds/);
+})();

test/mjsunit/wasm/type-based-optimizations.js

@@ -36,8 +36,8 @@ d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js");
           kExprLocalGet, 2, kGCPrefix, kExprRefTest, bottom1,
           kExprIf, kWasmVoid,
             // counter += ((bottom1) temp).field_2;
-            // TODO(manoskouk): Implement path-based type tracking so we can
-            // eliminate this check.
+            // Note: This cast should get optimized away with path-based type
+            // tracking.
             kExprLocalGet, 2, kGCPrefix, kExprRefCast, bottom1,
             kGCPrefix, kExprStructGet, bottom1, 2,
             kExprLocalGet, 3, kExprI32Add, kExprLocalSet, 3,

test/mjsunit/wasm/wasm-module-builder.js

@@ -1053,9 +1053,6 @@ class Binary {
   }
 
   emit_init_expr(expr) {
-    // TODO(manoskouk): This is redundant, remove it once we are confident we
-    // check everything.
-    checkExpr(expr);
     this.emit_bytes(expr);
     this.emit_u8(kExprEnd);
   }