[wasm][turbofan] Add effect output to trap conditionals

TrapIf and TrapUnless had an effect input, but not an effect output.
This is not canonical for Turbofan graphs. This CL puts them properly
into the effect chain.

Drive-by:
- Remove premature optimization in WasmGraphBuilder::TrapIfEq{32,64}.
- Change LoadFromObject to Load when loading from a stack slot.

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

src/compiler/branch-elimination.cc

@@ -175,17 +175,31 @@ Reduction BranchElimination::ReduceTrapConditional(Node* node) {
   if (branch_condition.IsSet()) {
     bool condition_value = branch_condition.is_true;
     if (condition_value == trapping_condition) {
-      // Special case: Trap directly inside a branch without sibling nodes.
-      // Replace the branch with the trap.
-      //    condition  control              condition  control
-      //     |   \      /                        \      /
-      //     |    Branch                          TrapIf
-      //     |    /    \            ==>             |
-      //     |  IfTrue IfFalse                 <subgraph2>
-      //     |  /        |
-      //    TrapIf   <subraph2>            Dead
-      //      |                              |
-      //   <subgraph1>                     <subgraph1>
+      // Special case: Trap directly below an IfTrue/IfFalse without sibling
+      // nodes. Replace the branch with the trap.
+      //
+      //   condition  effect  control
+      //     |     \ /      \ /
+      //     |      X        X
+      //     |     / \      / \
+      //     |    /   Branch   \
+      //     |   /    /   |     \
+      //     |  / IfTrue IfFalse \
+      //     | |   /      |       \
+      //     | |  /   control2  effect2
+      //     TrapIf
+      //       |   \
+      //  control1  effect1
+      //
+      //  --- becomes ---
+      //
+      //    condition  effect  control
+      //           \    |     /
+      //            \   |    /
+      //              TrapIf
+      //             /     \
+      //        control2  effect2
+      //
       // (and symmetrically for TrapUnless.)
       if (((trapping_condition &&
             control_input->opcode() == IrOpcode::kIfTrue) ||
@@ -201,10 +215,22 @@ Reduction BranchElimination::ReduceTrapConditional(Node* node) {
           }
           DCHECK_NOT_NULL(other_if_branch);
 
+          Node* effect_input = NodeProperties::GetEffectInput(node);
+          Node* other_effect = nullptr;
+          for (Node* use : effect_input->uses()) {
+            if (use != node) {
+              DCHECK_EQ(other_effect, nullptr);
+              other_effect = use;
+            }
+          }
+          DCHECK_NOT_NULL(other_effect);
+
           node->ReplaceInput(NodeProperties::FirstControlIndex(node),
                              NodeProperties::GetControlInput(branch));
           ReplaceWithValue(node, dead(), dead(), dead());
-          ReplaceWithValue(other_if_branch, node, node, node);
+          ReplaceWithValue(other_if_branch, dead(), dead(), node);
+          other_effect->ReplaceInput(
+              NodeProperties::FirstEffectIndex(other_effect), node);
           other_if_branch->Kill();
           control_input->Kill();
           branch->Kill();
@@ -215,14 +241,16 @@ Reduction BranchElimination::ReduceTrapConditional(Node* node) {
       // General case: This will always trap. Mark its outputs as dead and
       // connect it to graph()->end().
       ReplaceWithValue(node, dead(), dead(), dead());
-      Node* effect = NodeProperties::GetEffectInput(node);
-      Node* control = graph()->NewNode(common()->Throw(), effect, node);
+      Node* control = graph()->NewNode(common()->Throw(), node, node);
       NodeProperties::MergeControlToEnd(graph(), common(), control);
       Revisit(graph()->end());
       return Changed(node);
     } else {
-      // This will not trap, remove it.
-      return Replace(control_input);
+      // This will not trap, remove it by relaxing effect/control.
+      RelaxEffectsAndControls(node);
+      Node* control = NodeProperties::GetControlInput(node);
+      node->Kill();
+      return Replace(control);  // Irrelevant argument
     }
   }
   return UpdateStatesHelper(node, from_input, condition, node,

src/compiler/common-operator-reducer.cc

@@ -491,14 +491,17 @@ Reduction CommonOperatorReducer::ReduceTrapConditional(Node* trap) {
     // This will always trap. Mark its outputs as dead and connect it to
     // graph()->end().
     ReplaceWithValue(trap, dead(), dead(), dead());
-    Node* effect = NodeProperties::GetEffectInput(trap);
-    Node* control = graph()->NewNode(common()->Throw(), effect, trap);
+    Node* control = graph()->NewNode(common()->Throw(), trap, trap);
     NodeProperties::MergeControlToEnd(graph(), common(), control);
     Revisit(graph()->end());
     return Changed(trap);
   } else {
-    // This will not trap, remove it.
-    return Replace(NodeProperties::GetControlInput(trap));
+    // This will not trap, remove it by relaxing effect/control.
+    Node* control = NodeProperties::GetControlInput(trap);
+    ReplaceWithValue(trap, dead());
+    trap->Kill();
+    // The argument below is irrelevant, picked {control} for debugging.
+    return Replace(control);
   }
 }
 

src/compiler/common-operator.cc

@@ -768,7 +768,7 @@ struct CommonOperatorGlobalCache final {
               IrOpcode::kTrapIf,                         // opcode
               Operator::kFoldable | Operator::kNoThrow,  // properties
               "TrapIf",                                  // name
-              1, 1, 1, 0, 0, 1,                          // counts
+              1, 1, 1, 0, 1, 1,                          // counts
               trap_id) {}                                // parameter
   };
 #define CACHED_TRAP_IF(Trap) \
@@ -783,7 +783,7 @@ struct CommonOperatorGlobalCache final {
               IrOpcode::kTrapUnless,                     // opcode
               Operator::kFoldable | Operator::kNoThrow,  // properties
               "TrapUnless",                              // name
-              1, 1, 1, 0, 0, 1,                          // counts
+              1, 1, 1, 0, 1, 1,                          // counts
               trap_id) {}                                // parameter
   };
 #define CACHED_TRAP_UNLESS(Trap) \
@@ -1010,7 +1010,7 @@ const Operator* CommonOperatorBuilder::TrapIf(TrapId trap_id) {
       IrOpcode::kTrapIf,                         // opcode
       Operator::kFoldable | Operator::kNoThrow,  // properties
       "TrapIf",                                  // name
-      1, 1, 1, 0, 0, 1,                          // counts
+      1, 1, 1, 0, 1, 1,                          // counts
       trap_id);                                  // parameter
 }
 
@@ -1029,7 +1029,7 @@ const Operator* CommonOperatorBuilder::TrapUnless(TrapId trap_id) {
       IrOpcode::kTrapUnless,                     // opcode
       Operator::kFoldable | Operator::kNoThrow,  // properties
       "TrapUnless",                              // name
-      1, 1, 1, 0, 0, 1,                          // counts
+      1, 1, 1, 0, 1, 1,                          // counts
       trap_id);                                  // parameter
 }
 

src/compiler/memory-optimizer.cc

@@ -54,6 +54,8 @@ bool CanAllocate(const Node* node) {
     case IrOpcode::kStoreLane:
     case IrOpcode::kStoreToObject:
     case IrOpcode::kInitializeImmutableInObject:
+    case IrOpcode::kTrapIf:
+    case IrOpcode::kTrapUnless:
     case IrOpcode::kUnalignedLoad:
     case IrOpcode::kUnalignedStore:
     case IrOpcode::kUnreachable:

src/compiler/simplified-operator.cc

@@ -1155,7 +1155,7 @@ struct SimplifiedOperatorGlobalCache final {
   IsNullOperator kIsNull;
 
   struct IsNotNullOperator final : public Operator {
-    explicit IsNotNullOperator()
+    IsNotNullOperator()
         : Operator(IrOpcode::kIsNotNull, Operator::kPure, "IsNotNull", 1, 0, 0,
                    1, 0, 0) {}
   };
@@ -1173,7 +1173,7 @@ struct SimplifiedOperatorGlobalCache final {
         : Operator(
               IrOpcode::kAssertNotNull,
               Operator::kNoWrite | Operator::kNoThrow | Operator::kIdempotent,
-              "AssertNotNull", 1, 1, 1, 1, 0, 1) {}
+              "AssertNotNull", 1, 1, 1, 1, 1, 1) {}
   };
   AssertNotNullOperator kAssertNotNull;
 #endif

src/compiler/wasm-compiler.cc

@@ -1144,8 +1144,6 @@ Node* WasmGraphBuilder::AssertNotNull(Node* object,
 void WasmGraphBuilder::TrapIfEq32(wasm::TrapReason reason, Node* node,
                                   int32_t val,
                                   wasm::WasmCodePosition position) {
-  Int32Matcher m(node);
-  if (m.HasResolvedValue() && !m.Is(val)) return;
   if (val == 0) {
     TrapIfFalse(reason, node, position);
   } else {
@@ -1163,8 +1161,6 @@ void WasmGraphBuilder::ZeroCheck32(wasm::TrapReason reason, Node* node,
 void WasmGraphBuilder::TrapIfEq64(wasm::TrapReason reason, Node* node,
                                   int64_t val,
                                   wasm::WasmCodePosition position) {
-  Int64Matcher m(node);
-  if (m.HasResolvedValue() && !m.Is(val)) return;
   TrapIfTrue(reason, gasm_->Word64Equal(node, Int64Constant(val)), position);
 }
 
@@ -2314,18 +2310,17 @@ Node* WasmGraphBuilder::GetExceptionValues(Node* except_obj,
 Node* WasmGraphBuilder::BuildI32DivS(Node* left, Node* right,
                                      wasm::WasmCodePosition position) {
   ZeroCheck32(wasm::kTrapDivByZero, right, position);
-  Node* before = control();
+  Node* previous_effect = effect();
   Node* denom_is_m1;
   Node* denom_is_not_m1;
   BranchExpectFalse(gasm_->Word32Equal(right, Int32Constant(-1)), &denom_is_m1,
                     &denom_is_not_m1);
   SetControl(denom_is_m1);
   TrapIfEq32(wasm::kTrapDivUnrepresentable, left, kMinInt, position);
-  if (control() != denom_is_m1) {
-    SetControl(Merge(denom_is_not_m1, control()));
-  } else {
-    SetControl(before);
-  }
+  Node* merge = Merge(control(), denom_is_not_m1);
+  SetEffectControl(graph()->NewNode(mcgraph()->common()->EffectPhi(2), effect(),
+                                    previous_effect, merge),
+                   merge);
   return gasm_->Int32Div(left, right);
 }
 
@@ -2528,7 +2523,7 @@ Node* WasmGraphBuilder::BuildI64DivS(Node* left, Node* right,
                           MachineType::Int64(), wasm::kTrapDivByZero, position);
   }
   ZeroCheck64(wasm::kTrapDivByZero, right, position);
-  Node* before = control();
+  Node* previous_effect = effect();
   Node* denom_is_m1;
   Node* denom_is_not_m1;
   BranchExpectFalse(gasm_->Word64Equal(right, Int64Constant(-1)), &denom_is_m1,
@@ -2536,11 +2531,10 @@ Node* WasmGraphBuilder::BuildI64DivS(Node* left, Node* right,
   SetControl(denom_is_m1);
   TrapIfEq64(wasm::kTrapDivUnrepresentable, left,
              std::numeric_limits<int64_t>::min(), position);
-  if (control() != denom_is_m1) {
-    SetControl(Merge(denom_is_not_m1, control()));
-  } else {
-    SetControl(before);
-  }
+  Node* merge = Merge(control(), denom_is_not_m1);
+  SetEffectControl(graph()->NewNode(mcgraph()->common()->EffectPhi(2), effect(),
+                                    previous_effect, merge),
+                   merge);
   return gasm_->Int64Div(left, right);
 }
 
@@ -2598,7 +2592,7 @@ Node* WasmGraphBuilder::BuildDiv64Call(Node* left, Node* right,
 
   ZeroCheck32(trap_zero, call, position);
   TrapIfEq32(wasm::kTrapDivUnrepresentable, call, -1, position);
-  return gasm_->LoadFromObject(result_type, stack_slot, 0);
+  return gasm_->Load(result_type, stack_slot, 0);
 }
 
 Node* WasmGraphBuilder::IsNull(Node* object) {

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

@@ -119,16 +119,15 @@ Reduction WasmGCOperatorReducer::ReduceWasmTypeCast(Node* node) {
     Node* non_trapping_condition = object_type.type.is_nullable()
                                        ? gasm_.IsNull(object)
                                        : gasm_.Int32Constant(0);
-    Node* trap =
-        gasm_.TrapUnless(SetType(non_trapping_condition, wasm::kWasmI32),
-                         TrapId::kTrapIllegalCast);
+    gasm_.TrapUnless(SetType(non_trapping_condition, wasm::kWasmI32),
+                     TrapId::kTrapIllegalCast);
     // TODO(manoskouk): Improve the type when we have nullref.
     Node* null_node = gasm_.Null();
     ReplaceWithValue(
         node,
         SetType(null_node, wasm::ValueType::Ref(rtt_type.type.ref_index(),
                                                 wasm::kNullable)),
-        effect, trap);
+        gasm_.effect(), gasm_.control());
     node->Kill();
     return Replace(null_node);
   }

src/compiler/wasm-typer.cc

@@ -125,9 +125,8 @@ Reduction WasmTyper::Reduce(Node* node) {
                              previous_effect);
           node->ReplaceInput(NodeProperties::FirstControlIndex(node),
                              previous_control);
-          // We do not replace {object}'s effect input to {node} because {node}
-          // has no effect output.
           object->ReplaceInput(NodeProperties::FirstValueIndex(object), node);
+          object->ReplaceInput(NodeProperties::FirstEffectIndex(object), node);
           object->ReplaceInput(NodeProperties::FirstControlIndex(object), node);
           Revisit(object);
         }