[wasm-gc] Clean up a couple rough edges

Changes:
- Rename PassThrough -> Forward in function-body-decoder.
- Introduce IsHeapSubtypeOf in subtyping.
- Do not push a redundant bottom value in br_on_null, remove
  fallthrough. Also, improve code structure.
- Update a couple of comments.

Bug: v8:7748
Change-Id: I8d23cd3829c5504156ace595f8ac86c511c9f5e1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2611250
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72034}

src/compiler/wasm-compiler.cc

@@ -3122,6 +3122,7 @@ Node* WasmGraphBuilder::BuildLoadJumpTableOffsetFromExportedFunctionData(
   return BuildChangeSmiToIntPtr(jump_table_offset_smi);
 }
 
+// TODO(9495): Support CAPI function refs.
 Node* WasmGraphBuilder::BuildCallRef(uint32_t sig_index, Vector<Node*> args,
                                      Vector<Node*> rets,
                                      CheckForNull null_check,

src/wasm/baseline/liftoff-compiler.cc

@@ -4381,7 +4381,7 @@ class LiftoffCompiler {
     __ PushRegister(obj.type, obj_reg);
   }
 
-  void PassThrough(FullDecoder* decoder, const Value& from, Value* to) {
+  void Forward(FullDecoder* decoder, const Value& from, Value* to) {
     // Nothing to do here.
   }
 

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

@@ -1105,7 +1105,7 @@ struct ControlBase : public PcForErrors<validate> {
   F(RefCast, const Value& obj, const Value& rtt, Value* result)                \
   F(BrOnCast, const Value& obj, const Value& rtt, Value* result_on_branch,     \
     uint32_t depth)                                                            \
-  F(PassThrough, const Value& from, Value* to)
+  F(Forward, const Value& from, Value* to)
 
 // Generic Wasm bytecode decoder with utilities for decoding immediates,
 // lengths, etc.
@@ -2523,30 +2523,32 @@ class WasmFullDecoder : public WasmDecoder<validate> {
     Control* c = control_at(imm.depth);
     TypeCheckBranchResult check_result = TypeCheckBranch(c, true);
     switch (ref_object.type.kind()) {
-      // For bottom and non-nullable reference, simply forward the popped
-      // argument to the result.
       case ValueType::kBottom:
+        // We are in a polymorphic stack. No need to push an additional bottom
+        // value.
         DCHECK(check_result != kReachableBranch);
-        V8_FALLTHROUGH;
+        break;
       case ValueType::kRef: {
+        // Simply forward the popped argument to the result.
         Value* result = Push(ref_object.type);
         if (V8_LIKELY(check_result == kReachableBranch)) {
-          CALL_INTERFACE(PassThrough, ref_object, result);
+          CALL_INTERFACE(Forward, ref_object, result);
         }
         break;
       }
       case ValueType::kOptRef: {
-        // We need to Push the result value after calling BrOnNull on
-        // the interface. Therefore we must sync the ref_object and
-        // result nodes afterwards (in PassThrough).
         if (V8_LIKELY(check_result == kReachableBranch)) {
           CALL_INTERFACE_IF_REACHABLE(BrOnNull, ref_object, imm.depth);
-        }
-        Value* result =
-            Push(ValueType::Ref(ref_object.type.heap_type(), kNonNullable));
-        if (V8_LIKELY(check_result == kReachableBranch)) {
-          CALL_INTERFACE(PassThrough, ref_object, result);
+          Value* result =
+              Push(ValueType::Ref(ref_object.type.heap_type(), kNonNullable));
+          // The result of br_on_null has the same value as the argument (but a
+          // non-nullable type).
+          CALL_INTERFACE(Forward, ref_object, result);
           c->br_merge()->reached = true;
+        } else {
+          // Even in non-reachable code, we need to push a value of the correct
+          // type to the stack.
+          Push(ValueType::Ref(ref_object.type.heap_type(), kNonNullable));
         }
         break;
       }
@@ -2897,7 +2899,7 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         // We are in unreachable code. Forward the bottom value.
       case ValueType::kRef: {
         Value* result = Push(value.type);
-        CALL_INTERFACE_IF_REACHABLE(PassThrough, value, result);
+        CALL_INTERFACE_IF_REACHABLE(Forward, value, result);
         return 1;
       }
       case ValueType::kOptRef: {
@@ -4006,13 +4008,9 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         if (parent.type.is_bottom()) {
           Push(kWasmBottom);
         } else {
-          // TODO(7748): Consider exposing "IsSubtypeOfHeap(HeapType t1, t2)" so
-          // we can avoid creating (ref heaptype) wrappers here.
           if (!VALIDATE(parent.type.is_rtt() &&
-                        IsSubtypeOf(ValueType::Ref(imm.type, kNonNullable),
-                                    ValueType::Ref(parent.type.heap_type(),
-                                                   kNonNullable),
-                                    this->module_))) {
+                        IsHeapSubtypeOf(imm.type, parent.type.heap_type(),
+                                        this->module_))) {
             PopTypeError(0, parent,
                          "rtt for a supertype of type " + imm.type.name());
             return 0;
@@ -4043,9 +4041,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         if (!VALIDATE(this->ok())) return 0;
 
         // The static type of {obj} must be a supertype of the {rtt}'s type.
-        if (!VALIDATE(IsSubtypeOf(ValueType::Ref(rtt_type.type, kNonNullable),
-                                  ValueType::Ref(obj_type.type, kNonNullable),
-                                  this->module_))) {
+        if (!VALIDATE(
+                IsHeapSubtypeOf(rtt_type.type, obj_type.type, this->module_))) {
           this->DecodeError(
               "ref.test: immediate rtt type %s is not a subtype of immediate "
               "object type %s",
@@ -4073,9 +4070,8 @@ class WasmFullDecoder : public WasmDecoder<validate> {
         len += rtt_type.length;
         if (!VALIDATE(this->ok())) return 0;
 
-        if (!VALIDATE(IsSubtypeOf(ValueType::Ref(rtt_type.type, kNonNullable),
-                                  ValueType::Ref(obj_type.type, kNonNullable),
-                                  this->module_))) {
+        if (!VALIDATE(
+                IsHeapSubtypeOf(rtt_type.type, obj_type.type, this->module_))) {
           this->DecodeError(
               "ref.test: immediate rtt type %s is not a subtype of immediate "
               "object type %s",
@@ -4115,11 +4111,9 @@ class WasmFullDecoder : public WasmDecoder<validate> {
           return 0;
         }
         // The static type of {obj} must be a supertype of {rtt}'s type.
-        if (!VALIDATE(
-                rtt.type.is_bottom() || obj.type.is_bottom() ||
-                IsSubtypeOf(ValueType::Ref(rtt.type.heap_type(), kNonNullable),
-                            ValueType::Ref(obj.type.heap_type(), kNonNullable),
-                            this->module_))) {
+        if (!VALIDATE(rtt.type.is_bottom() || obj.type.is_bottom() ||
+                      IsHeapSubtypeOf(rtt.type.heap_type(),
+                                      obj.type.heap_type(), this->module_))) {
           PopTypeError(1, rtt, obj.type);
           return 0;
         }

src/wasm/graph-builder-interface.cc

@@ -894,7 +894,7 @@ class WasmGraphBuildingInterface {
     SetEnv(no_match_env);
   }
 
-  void PassThrough(FullDecoder* decoder, const Value& from, Value* to) {
+  void Forward(FullDecoder* decoder, const Value& from, Value* to) {
     to->node = from.node;
   }
 
@@ -1139,6 +1139,8 @@ class WasmGraphBuildingInterface {
     TFNode* effect_inputs[] = {effect(), control()};
     builder_->SetEffect(builder_->EffectPhi(1, effect_inputs));
     builder_->TerminateLoop(effect(), control());
+    // Doing a preprocessing pass to analyze loop assignments seems to pay off
+    // compared to reallocating Nodes when rearranging Phis in Goto.
     BitVector* assigned = WasmDecoder<validate>::AnalyzeLoopAssignment(
         decoder, decoder->pc(), decoder->num_locals(), decoder->zone());
     if (decoder->failed()) return;

src/wasm/wasm-subtyping.h

@@ -69,6 +69,14 @@ V8_INLINE bool IsSubtypeOf(ValueType subtype, ValueType supertype,
   return IsSubtypeOfImpl(subtype, supertype, module, module);
 }
 
+// We have this function call IsSubtypeOf instead of the opposite because type
+// checks are much more common than heap type checks.
+V8_INLINE bool IsHeapSubtypeOf(HeapType subtype, HeapType supertype,
+                               const WasmModule* module) {
+  return IsSubtypeOf(ValueType::Ref(subtype, kNonNullable),
+                     ValueType::Ref(supertype, kNonNullable), module);
+}
+
 // Returns the weakest type that is a subtype of both a and b
 // (which is currently always one of a, b, or kWasmBottom).
 // TODO(manoskouk): Update this once we have settled on a type system for

test/cctest/wasm/test-gc.cc

@@ -1122,8 +1122,6 @@ WASM_COMPILED_EXEC_TEST(BasicI31) {
   const byte kUnsigned = tester.DefineFunction(
       tester.sigs.i_i(), {},
       {WASM_I31_GET_U(WASM_I31_NEW(WASM_LOCAL_GET(0))), kExprEnd});
-  // TODO(7748): Support (rtt.canon i31), and add a test like:
-  // (ref.test (i31.new ...) (rtt.canon i31)).
   tester.CompileModule();
   tester.CheckResult(kSigned, 123, 123);
   tester.CheckResult(kUnsigned, 123, 123);