[wasm] Fix effect chain, enable its verification

This CL fixes all spots where wasm Turbofan code did not satisfy the
invariant that all nodes with effect outputs are connected to another
node. Also, it enables the related verification for wasm code.

Drive-by:
- Simplify how stack checks are removed during loop unrolling.
- Fix a test declaration in test-gc.cc.

Change-Id: Id32af8584ba0ec281f4bf7757bd2915e6d8bf443
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3676862
Commit-Queue: Manos Koukoutos <manoskouk@chromium.org>
Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80854}

src/compiler/int64-lowering.cc

@@ -639,33 +639,15 @@ void Int64Lowering::LowerNode(Node* node) {
     case IrOpcode::kBitcastInt64ToFloat64: {
       DCHECK_EQ(1, node->InputCount());
       Node* input = node->InputAt(0);
-      Node* stack_slot = graph()->NewNode(
-          machine()->StackSlot(MachineRepresentation::kWord64));
-
-      Node* store_high_word = graph()->NewNode(
-          machine()->Store(
-              StoreRepresentation(MachineRepresentation::kWord32,
-                                  WriteBarrierKind::kNoWriteBarrier)),
-          stack_slot,
-          graph()->NewNode(
-              common()->Int32Constant(kInt64UpperHalfMemoryOffset)),
-          GetReplacementHigh(input), graph()->start(), graph()->start());
-
-      Node* store_low_word = graph()->NewNode(
-          machine()->Store(
-              StoreRepresentation(MachineRepresentation::kWord32,
-                                  WriteBarrierKind::kNoWriteBarrier)),
-          stack_slot,
-          graph()->NewNode(
-              common()->Int32Constant(kInt64LowerHalfMemoryOffset)),
-          GetReplacementLow(input), store_high_word, graph()->start());
 
-      Node* load =
-          graph()->NewNode(machine()->Load(MachineType::Float64()), stack_slot,
-                           graph()->NewNode(common()->Int32Constant(0)),
-                           store_low_word, graph()->start());
+      Node* high_half =
+          graph()->NewNode(machine()->Float64InsertHighWord32(),
+                           graph()->NewNode(common()->Float64Constant(0.0)),
+                           GetReplacementHigh(input));
+      Node* result = graph()->NewNode(machine()->Float64InsertLowWord32(),
+                                      high_half, GetReplacementLow(input));
 
-      ReplaceNode(node, load, nullptr);
+      ReplaceNode(node, result, nullptr);
       break;
     }
     case IrOpcode::kBitcastFloat64ToInt64: {
@@ -674,26 +656,12 @@ void Int64Lowering::LowerNode(Node* node) {
       if (HasReplacementLow(input)) {
         input = GetReplacementLow(input);
       }
-      Node* stack_slot = graph()->NewNode(
-          machine()->StackSlot(MachineRepresentation::kWord64));
-      Node* store = graph()->NewNode(
-          machine()->Store(
-              StoreRepresentation(MachineRepresentation::kFloat64,
-                                  WriteBarrierKind::kNoWriteBarrier)),
-          stack_slot, graph()->NewNode(common()->Int32Constant(0)), input,
-          graph()->start(), graph()->start());
 
-      Node* high_node = graph()->NewNode(
-          machine()->Load(MachineType::Int32()), stack_slot,
-          graph()->NewNode(
-              common()->Int32Constant(kInt64UpperHalfMemoryOffset)),
-          store, graph()->start());
+      Node* low_node =
+          graph()->NewNode(machine()->Float64ExtractLowWord32(), input);
+      Node* high_node =
+          graph()->NewNode(machine()->Float64ExtractHighWord32(), input);
 
-      Node* low_node = graph()->NewNode(
-          machine()->Load(MachineType::Int32()), stack_slot,
-          graph()->NewNode(
-              common()->Int32Constant(kInt64LowerHalfMemoryOffset)),
-          store, graph()->start());
       ReplaceNode(node, low_node, high_node);
       break;
     }

src/compiler/loop-unrolling.cc

@@ -61,36 +61,17 @@ void UnrollLoop(Node* loop_node, ZoneUnorderedSet<Node*>* loop, uint32_t depth,
         if (stack_check->opcode() != IrOpcode::kStackPointerGreaterThan) {
           break;
         }
+        // Replace value uses of the stack check with {true}, and remove the
+        // stack check from the effect chain.
         FOREACH_COPY_INDEX(i) {
-          COPY(node, i)->ReplaceInput(0,
-                                      graph->NewNode(common->Int32Constant(1)));
-        }
-        for (Node* use : stack_check->uses()) {
-          if (use->opcode() == IrOpcode::kEffectPhi) {
-            // We now need to remove stack check and the related function call
-            // from the effect chain.
-            // The effect chain looks like this (* stand for irrelevant nodes):
-            //
-            // {replacing_effect} (effect before stack check)
-            //   *  *  |  *
-            //   |  |  |  |
-            // ( LoadFromObject )
-            //   |  |
-            // {stack_check}
-            // |   |
-            // |   *
-            // |
-            // | * *
-            // | | |
-            // {use}: EffectPhi (stack check effect that we need to replace)
-            DCHECK_EQ(use->opcode(), IrOpcode::kEffectPhi);
-            DCHECK_EQ(NodeProperties::GetEffectInput(use), stack_check);
-            DCHECK_EQ(NodeProperties::GetEffectInput(stack_check)->opcode(),
-                      IrOpcode::kLoadFromObject);
-            Node* replacing_effect = NodeProperties::GetEffectInput(
-                NodeProperties::GetEffectInput(stack_check));
-            FOREACH_COPY_INDEX(i) {
-              COPY(use, i)->ReplaceUses(COPY(replacing_effect, i));
+          for (Edge use_edge : COPY(stack_check, i)->use_edges()) {
+            if (NodeProperties::IsValueEdge(use_edge)) {
+              use_edge.UpdateTo(graph->NewNode(common->Int32Constant(1)));
+            } else if (NodeProperties::IsEffectEdge(use_edge)) {
+              use_edge.UpdateTo(
+                  NodeProperties::GetEffectInput(COPY(stack_check, i)));
+            } else {
+              UNREACHABLE();
             }
           }
         }

src/compiler/verifier.cc

@@ -153,8 +153,7 @@ void Verifier::Visitor::Check(Node* node, const AllNodes& all) {
 
   // If this node has any effect outputs, make sure that it is
   // consumed as an effect input somewhere else.
-  // TODO(mvstanton): support this kind of verification for Wasm compiles, too.
-  if (code_type != kWasm && node->op()->EffectOutputCount() > 0) {
+  if (node->op()->EffectOutputCount() > 0) {
 #ifdef DEBUG
     int effect_edges = 0;
     for (Edge edge : node->use_edges()) {

src/compiler/wasm-compiler.cc

@@ -403,19 +403,22 @@ void WasmGraphBuilder::StackCheck(
 
   SetEffectControl(call, if_false);
 
+  // We only need to refresh the size of a shared memory, as its start can never
+  // change.
+  // We handle caching of the instance cache nodes manually, and we may reload
+  // them in contexts where load elimination would eliminate the reload.
+  // Therefore, we use plain Load nodes which are not subject to load
+  // elimination.
+  Node* new_memory_size = shared_memory_instance_cache == nullptr
+                              ? nullptr
+                              : LOAD_INSTANCE_FIELD_NO_ELIMINATION(
+                                    MemorySize, MachineType::UintPtr());
+
   Node* merge = Merge(if_true, control());
   Node* ephi_inputs[] = {check, effect(), merge};
   Node* ephi = EffectPhi(2, ephi_inputs);
 
-  // We only need to refresh the size of a shared memory, as its start can never
-  // change.
   if (shared_memory_instance_cache != nullptr) {
-    // We handle caching of the instance cache nodes manually, and we may reload
-    // them in contexts where load elimination would eliminate the reload.
-    // Therefore, we use plain Load nodes which are not subject to load
-    // elimination.
-    Node* new_memory_size =
-        LOAD_INSTANCE_FIELD_NO_ELIMINATION(MemorySize, MachineType::UintPtr());
     shared_memory_instance_cache->mem_size = CreateOrMergeIntoPhi(
         MachineType::PointerRepresentation(), merge,
         shared_memory_instance_cache->mem_size, new_memory_size);
@@ -2664,9 +2667,10 @@ Node* WasmGraphBuilder::BuildWasmCall(const wasm::FunctionSig* sig,
   const Operator* op = mcgraph()->common()->Call(call_descriptor);
   Node* call =
       BuildCallNode(sig, args, position, instance_node, op, frame_state);
-  // TODO(manoskouk): If we have kNoThrow calls, do not set them as control.
-  DCHECK_GT(call->op()->ControlOutputCount(), 0);
-  SetControl(call);
+  // TODO(manoskouk): These assume the call has control and effect outputs.
+  DCHECK_GT(op->ControlOutputCount(), 0);
+  DCHECK_GT(op->EffectOutputCount(), 0);
+  SetEffectControl(call);
 
   size_t ret_count = sig->return_count();
   if (ret_count == 0) return call;  // No return value.
@@ -7245,11 +7249,11 @@ class WasmWrapperGraphBuilder : public WasmGraphBuilder {
         graph()->NewNode(mcgraph()->common()->IfException(), call, call);
 
     // Handle exception: return it.
-    SetControl(if_exception);
+    SetEffectControl(if_exception);
     Return(if_exception);
 
     // Handle success: store the return value(s).
-    SetControl(if_success);
+    SetEffectControl(call, if_success);
     pos = 0;
     offset = 0;
     for (wasm::ValueType type : sig_->returns()) {

test/cctest/wasm/test-gc.cc

@@ -1760,7 +1760,7 @@ WASM_COMPILED_EXEC_TEST(TrivialAbstractCasts) {
   tester.CheckHasThrown(kAsArrayUnrelatedNonNullable);
 }
 
-WASM_EXEC_TEST(NoDepthRtt) {
+WASM_COMPILED_EXEC_TEST(NoDepthRtt) {
   WasmGCTester tester(execution_tier);
 
   const byte type_index = tester.DefineStruct({F(wasm::kWasmI32, true)});

test/cctest/wasm/wasm-run-utils.cc

@@ -473,8 +473,15 @@ void WasmFunctionWrapper::Init(CallDescriptor* call_descriptor,
 
   parameters[parameter_count++] = effect;
   parameters[parameter_count++] = graph()->start();
-  Node* call = graph()->NewNode(common()->Call(call_descriptor),
-                                parameter_count, parameters);
+  const compiler::Operator* call_op = common()->Call(call_descriptor);
+  // The following code assumes the call node has effect and control inputs and
+  // outputs.
+  DCHECK_GT(call_op->EffectInputCount(), 0);
+  DCHECK_GT(call_op->EffectOutputCount(), 0);
+  DCHECK_GT(call_op->ControlInputCount(), 0);
+  DCHECK_GT(call_op->ControlOutputCount(), 0);
+
+  Node* call = graph()->NewNode(call_op, parameter_count, parameters);
 
   if (!return_type.IsNone()) {
     effect = graph()->NewNode(
@@ -483,14 +490,13 @@ void WasmFunctionWrapper::Init(CallDescriptor* call_descriptor,
             compiler::WriteBarrierKind::kNoWriteBarrier)),
         graph()->NewNode(common()->Parameter(param_types.length()),
                          graph()->start()),
-        graph()->NewNode(common()->Int32Constant(0)), call, effect,
-        graph()->start());
+        graph()->NewNode(common()->Int32Constant(0)), call, call, call);
   }
   Node* zero = graph()->NewNode(common()->Int32Constant(0));
   Node* r = graph()->NewNode(
       common()->Return(), zero,
       graph()->NewNode(common()->Int32Constant(WASM_WRAPPER_RETURN_VALUE)),
-      effect, graph()->start());
+      effect, call);
   graph()->SetEnd(graph()->NewNode(common()->End(1), r));
 }
 

test/unittests/compiler/int64-lowering-unittest.cc

@@ -10,6 +10,7 @@
 #include "src/compiler/common-operator.h"
 #include "src/compiler/linkage.h"
 #include "src/compiler/machine-operator.h"
+#include "src/compiler/node-matchers.h"
 #include "src/compiler/node-properties.h"
 #include "src/compiler/node.h"
 #include "src/compiler/wasm-compiler.h"
@@ -861,65 +862,35 @@ TEST_F(Int64LoweringTest, I64UConvertI32_2) {
 }
 
 TEST_F(Int64LoweringTest, F64ReinterpretI64) {
+  int64_t value = 0x0123456789abcdef;
   LowerGraph(graph()->NewNode(machine()->BitcastInt64ToFloat64(),
-                              Int64Constant(value(0))),
+                              Int64Constant(value)),
              MachineRepresentation::kFloat64);
-
-  Capture<Node*> stack_slot_capture;
-  Matcher<Node*> stack_slot_matcher =
-      IsStackSlot(StackSlotRepresentation(sizeof(int64_t), 0));
-
-  Capture<Node*> store_capture;
-  Matcher<Node*> store_matcher =
-      IsStore(StoreRepresentation(MachineRepresentation::kWord32,
-                                  WriteBarrierKind::kNoWriteBarrier),
-              AllOf(CaptureEq(&stack_slot_capture), stack_slot_matcher),
-              IsInt32Constant(kInt64LowerHalfMemoryOffset),
-              IsInt32Constant(low_word_value(0)),
-              IsStore(StoreRepresentation(MachineRepresentation::kWord32,
-                                          WriteBarrierKind::kNoWriteBarrier),
-                      AllOf(CaptureEq(&stack_slot_capture), stack_slot_matcher),
-                      IsInt32Constant(kInt64UpperHalfMemoryOffset),
-                      IsInt32Constant(high_word_value(0)), start(), start()),
-              start());
-
-  EXPECT_THAT(
-      graph()->end()->InputAt(1),
-      IsReturn(IsLoad(MachineType::Float64(),
-                      AllOf(CaptureEq(&stack_slot_capture), stack_slot_matcher),
-                      IsInt32Constant(0),
-                      AllOf(CaptureEq(&store_capture), store_matcher), start()),
-               start(), start()));
+  Node* ret = graph()->end()->InputAt(1);
+  EXPECT_EQ(ret->opcode(), IrOpcode::kReturn);
+  Node* ret_value = ret->InputAt(1);
+  EXPECT_EQ(ret_value->opcode(), IrOpcode::kFloat64InsertLowWord32);
+  Node* high_half = ret_value->InputAt(0);
+  EXPECT_EQ(high_half->opcode(), IrOpcode::kFloat64InsertHighWord32);
+  Node* low_half_bits = ret_value->InputAt(1);
+  Int32Matcher m1(low_half_bits);
+  EXPECT_TRUE(m1.Is(static_cast<int32_t>(value & 0xFFFFFFFF)));
+  Node* high_half_bits = high_half->InputAt(1);
+  Int32Matcher m2(high_half_bits);
+  EXPECT_TRUE(m2.Is(static_cast<int32_t>(value >> 32)));
 }
 
 TEST_F(Int64LoweringTest, I64ReinterpretF64) {
-  LowerGraph(
-      graph()->NewNode(machine()->BitcastFloat64ToInt64(),
-                       Float64Constant(base::bit_cast<double>(value(0)))),
-      MachineRepresentation::kWord64);
-
-  Capture<Node*> stack_slot;
-  Matcher<Node*> stack_slot_matcher =
-      IsStackSlot(StackSlotRepresentation(sizeof(int64_t), 0));
-
-  Capture<Node*> store;
-  Matcher<Node*> store_matcher = IsStore(
-      StoreRepresentation(MachineRepresentation::kFloat64,
-                          WriteBarrierKind::kNoWriteBarrier),
-      AllOf(CaptureEq(&stack_slot), stack_slot_matcher), IsInt32Constant(0),
-      IsFloat64Constant(base::bit_cast<double>(value(0))), start(), start());
-
-  EXPECT_THAT(
-      graph()->end()->InputAt(1),
-      IsReturn2(IsLoad(MachineType::Int32(),
-                       AllOf(CaptureEq(&stack_slot), stack_slot_matcher),
-                       IsInt32Constant(kInt64LowerHalfMemoryOffset),
-                       AllOf(CaptureEq(&store), store_matcher), start()),
-                IsLoad(MachineType::Int32(),
-                       AllOf(CaptureEq(&stack_slot), stack_slot_matcher),
-                       IsInt32Constant(kInt64UpperHalfMemoryOffset),
-                       AllOf(CaptureEq(&store), store_matcher), start()),
-                start(), start()));
+  double value = 1234.5678;
+  LowerGraph(graph()->NewNode(machine()->BitcastFloat64ToInt64(),
+                              Float64Constant(value)),
+             MachineRepresentation::kWord64);
+  Node* ret = graph()->end()->InputAt(1);
+  EXPECT_EQ(ret->opcode(), IrOpcode::kReturn);
+  Node* ret_value_low = ret->InputAt(1);
+  EXPECT_EQ(ret_value_low->opcode(), IrOpcode::kFloat64ExtractLowWord32);
+  Node* ret_value_high = ret->InputAt(2);
+  EXPECT_EQ(ret_value_high->opcode(), IrOpcode::kFloat64ExtractHighWord32);
 }
 
 TEST_F(Int64LoweringTest, Dfs) {