Reland "[turbofan] Enable --verify-simplified-lowering in debug"

This reverts commit aaedd8b7.

Changes in the reland:
The inital problem was caused by nodes that were removed during SL
because they are no-ops but have an effect on typing (in the repro, this
was e.g. PlainPrimitiveToNumber). The reland introdocues a new operator
SLVerifierHint that is used exclusively in SL to provide hints to the
verifier and that solves this problem. SLVerifierHint also replaces the
previous use of TypeGuard to type constant nodes for the verifier.

Bug: v8:12619, chromium:1302572
Change-Id: I0957645c03d8b7c26cd6d630a1ecbd0a6a8223ce
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3512574Reviewed-by: Tobias Tebbi <tebbi@chromium.org>
Commit-Queue: Nico Hartmann <nicohartmann@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79564}

src/compiler/common-operator.cc

@@ -408,6 +408,39 @@ IfValueParameters const& IfValueParametersOf(const Operator* op) {
   return OpParameter<IfValueParameters>(op);
 }
 
+V8_EXPORT_PRIVATE bool operator==(const SLVerifierHintParameters& p1,
+                                  const SLVerifierHintParameters& p2) {
+  return p1.semantics() == p2.semantics() &&
+         p1.override_output_type() == p2.override_output_type();
+}
+
+size_t hash_value(const SLVerifierHintParameters& p) {
+  return base::hash_combine(
+      p.semantics(),
+      p.override_output_type() ? hash_value(*p.override_output_type()) : 0);
+}
+
+V8_EXPORT_PRIVATE std::ostream& operator<<(std::ostream& out,
+                                           const SLVerifierHintParameters& p) {
+  if (p.semantics()) {
+    p.semantics()->PrintTo(out);
+  } else {
+    out << "nullptr";
+  }
+  out << ", ";
+  if (const auto& t = p.override_output_type()) {
+    t->PrintTo(out);
+  } else {
+    out << ", nullopt";
+  }
+  return out;
+}
+
+const SLVerifierHintParameters& SLVerifierHintParametersOf(const Operator* op) {
+  DCHECK_EQ(op->opcode(), IrOpcode::kSLVerifierHint);
+  return OpParameter<SLVerifierHintParameters>(op);
+}
+
 #define COMMON_CACHED_OP_LIST(V)                          \
   V(Plug, Operator::kNoProperties, 0, 0, 0, 1, 0, 0)      \
   V(Dead, Operator::kFoldable, 0, 0, 0, 1, 1, 1)          \
@@ -892,6 +925,14 @@ const Operator* CommonOperatorBuilder::StaticAssert(const char* source) {
       1, 0, source);
 }
 
+const Operator* CommonOperatorBuilder::SLVerifierHint(
+    const Operator* semantics,
+    const base::Optional<Type>& override_output_type) {
+  return zone()->New<Operator1<SLVerifierHintParameters>>(
+      IrOpcode::kSLVerifierHint, Operator::kNoProperties, "SLVerifierHint", 1,
+      0, 0, 1, 0, 0, SLVerifierHintParameters(semantics, override_output_type));
+}
+
 const Operator* CommonOperatorBuilder::Branch(BranchHint hint) {
 #define CACHED_BRANCH(Hint)                 \
   if (hint == BranchHint::k##Hint) {        \

src/compiler/common-operator.h

@@ -427,6 +427,33 @@ const StringConstantBase* StringConstantBaseOf(const Operator* op)
 
 const char* StaticAssertSourceOf(const Operator* op);
 
+class SLVerifierHintParameters final {
+ public:
+  explicit SLVerifierHintParameters(const Operator* semantics,
+                                    base::Optional<Type> override_output_type)
+      : semantics_(semantics), override_output_type_(override_output_type) {}
+
+  const Operator* semantics() const { return semantics_; }
+  const base::Optional<Type>& override_output_type() const {
+    return override_output_type_;
+  }
+
+ private:
+  const Operator* semantics_;
+  base::Optional<Type> override_output_type_;
+};
+
+V8_EXPORT_PRIVATE bool operator==(const SLVerifierHintParameters& p1,
+                                  const SLVerifierHintParameters& p2);
+
+size_t hash_value(const SLVerifierHintParameters& p);
+
+V8_EXPORT_PRIVATE std::ostream& operator<<(std::ostream& out,
+                                           const SLVerifierHintParameters& p);
+
+V8_EXPORT_PRIVATE const SLVerifierHintParameters& SLVerifierHintParametersOf(
+    const Operator* op) V8_WARN_UNUSED_RESULT;
+
 // Interface for building common operators that can be used at any level of IR,
 // including JavaScript, mid-level, and low-level.
 class V8_EXPORT_PRIVATE CommonOperatorBuilder final
@@ -445,6 +472,12 @@ class V8_EXPORT_PRIVATE CommonOperatorBuilder final
   const Operator* DeadValue(MachineRepresentation rep);
   const Operator* Unreachable();
   const Operator* StaticAssert(const char* source);
+  // SLVerifierHint is used only during SimplifiedLowering. It may be introduced
+  // during lowering to provide additional hints for the verifier. These nodes
+  // are removed at the end of SimplifiedLowering after verification.
+  const Operator* SLVerifierHint(
+      const Operator* semantics,
+      const base::Optional<Type>& override_output_type);
   const Operator* End(size_t control_input_count);
   const Operator* Branch(BranchHint = BranchHint::kNone);
   const Operator* IfTrue();

src/compiler/opcodes.h

@@ -83,6 +83,7 @@
   V(DeadValue)            \
   V(Dead)                 \
   V(Plug)                 \
+  V(SLVerifierHint)       \
   V(StaticAssert)
 
 // Opcodes for JavaScript operators.

src/compiler/representation-change.cc

@@ -844,7 +844,7 @@ Node* RepresentationChanger::GetWord32RepresentationFor(
             use_info.type_check() == TypeCheckKind::kNumberOrOddball ||
             use_info.type_check() == TypeCheckKind::kArrayIndex) &&
            IsInt32Double(fv))) {
-        return InsertTypeGuardForVerifier(NodeProperties::GetType(node),
+        return InsertTypeOverrideForVerifier(NodeProperties::GetType(node),
                                              MakeTruncatedInt32Constant(fv));
       }
       break;
@@ -1109,7 +1109,7 @@ Node* RepresentationChanger::GetWord64RepresentationFor(
         if (base::IsValueInRangeForNumericType<int64_t>(fv)) {
           int64_t const iv = static_cast<int64_t>(fv);
           if (static_cast<double>(iv) == fv) {
-            return InsertTypeGuardForVerifier(NodeProperties::GetType(node),
+            return InsertTypeOverrideForVerifier(NodeProperties::GetType(node),
                                                  jsgraph()->Int64Constant(iv));
           }
         }
@@ -1121,7 +1121,7 @@ Node* RepresentationChanger::GetWord64RepresentationFor(
       if (m.HasResolvedValue() && m.Ref(broker_).IsBigInt() &&
           use_info.truncation().IsUsedAsWord64()) {
         BigIntRef bigint = m.Ref(broker_).AsBigInt();
-        return InsertTypeGuardForVerifier(
+        return InsertTypeOverrideForVerifier(
             NodeProperties::GetType(node),
             jsgraph()->Int64Constant(static_cast<int64_t>(bigint.AsUint64())));
       }
@@ -1571,14 +1571,13 @@ Node* RepresentationChanger::InsertCheckedFloat64ToInt32(
       node, simplified()->CheckedFloat64ToInt32(check, feedback), use_node);
 }
 
-Node* RepresentationChanger::InsertTypeGuardForVerifier(const Type& type,
+Node* RepresentationChanger::InsertTypeOverrideForVerifier(const Type& type,
                                                            Node* node) {
   if (verification_enabled()) {
     DCHECK(!type.IsInvalid());
-    node = jsgraph()->graph()->NewNode(jsgraph()->common()->TypeGuard(type),
-                                       node, jsgraph()->graph()->start(),
-                                       jsgraph()->graph()->start());
-    verifier_->RecordTypeGuard(node);
+    node = jsgraph()->graph()->NewNode(
+        jsgraph()->common()->SLVerifierHint(nullptr, type), node);
+    verifier_->RecordHint(node);
   }
   return node;
 }

src/compiler/representation-change.h

@@ -405,7 +405,7 @@ class V8_EXPORT_PRIVATE RepresentationChanger final {
   Node* InsertTruncateInt64ToInt32(Node* node);
   Node* InsertUnconditionalDeopt(Node* node, DeoptimizeReason reason,
                                  const FeedbackSource& feedback = {});
-  Node* InsertTypeGuardForVerifier(const Type& type, Node* node);
+  Node* InsertTypeOverrideForVerifier(const Type& type, Node* node);
 
   JSGraph* jsgraph() const { return jsgraph_; }
   Isolate* isolate() const;

src/compiler/simplified-lowering-verifier.cc

@@ -22,11 +22,8 @@ Truncation LeastGeneralTruncation(const Truncation& t1, const Truncation& t2,
   return LeastGeneralTruncation(LeastGeneralTruncation(t1, t2), t3);
 }
 
-void SimplifiedLoweringVerifier::CheckAndSet(Node* node, const Type& type,
-                                             const Truncation& trunc) {
-  DCHECK(!type.IsInvalid());
-
-  if (NodeProperties::IsTyped(node)) {
+void SimplifiedLoweringVerifier::CheckType(Node* node, const Type& type) {
+  CHECK(NodeProperties::IsTyped(node));
   Type node_type = NodeProperties::GetType(node);
   if (!type.Is(node_type)) {
     std::ostringstream type_str;
@@ -40,8 +37,19 @@ void SimplifiedLoweringVerifier::CheckAndSet(Node* node, const Type& type,
         type_str.str().c_str(), node->id(), node->op()->mnemonic(),
         node_type_str.str().c_str());
   }
+}
+
+void SimplifiedLoweringVerifier::CheckAndSet(Node* node, const Type& type,
+                                             const Truncation& trunc) {
+  DCHECK(!type.IsInvalid());
+
+  if (NodeProperties::IsTyped(node)) {
+    CheckType(node, type);
   } else {
-    NodeProperties::SetType(node, type);
+    // We store the type inferred by the verification pass. We do not update
+    // the node's type directly, because following phases might encounter
+    // unsound types as long as the verification is not complete.
+    SetType(node, type);
   }
   SetTruncation(node, GeneralizeTruncation(trunc, type));
 }
@@ -188,20 +196,7 @@ void SimplifiedLoweringVerifier::VisitNode(Node* node,
       break;
     }
     case IrOpcode::kTypeGuard: {
-      Type input_type = Type::Any();
-      if (is_recorded_type_guard(node)) {
-        // If this TypeGuard is recorded, it means that it has been introduced
-        // during lowering to provide type information for nodes that cannot be
-        // typed directly (e.g. constants), so we cannot assume the input node
-        // is typed.
-        if (NodeProperties::IsTyped(node->InputAt(0))) {
-          input_type = InputType(node, 0);
-        }
-      } else {
-        input_type = InputType(node, 0);
-      }
-      Type output_type = op_typer.TypeTypeGuard(node->op(), input_type);
-
+      Type output_type = op_typer.TypeTypeGuard(node->op(), InputType(node, 0));
       // TypeGuard has no effect on trunction, but the restricted type may help
       // generalize it.
       CheckAndSet(node, output_type, InputTruncation(node, 0));
@@ -239,6 +234,30 @@ void SimplifiedLoweringVerifier::VisitNode(Node* node,
       }
       break;
     }
+    case IrOpcode::kSLVerifierHint: {
+      Type output_type = InputType(node, 0);
+      Truncation output_trunc = InputTruncation(node, 0);
+      const auto& p = SLVerifierHintParametersOf(node->op());
+
+      if (const Operator* semantics = p.semantics()) {
+        switch (semantics->opcode()) {
+          case IrOpcode::kPlainPrimitiveToNumber:
+            output_type = op_typer.ToNumber(output_type);
+            break;
+          default:
+            UNREACHABLE();
+        }
+        CheckType(node, output_type);
+      }
+
+      if (p.override_output_type()) {
+        output_type = *p.override_output_type();
+      }
+
+      SetType(node, output_type);
+      SetTruncation(node, GeneralizeTruncation(output_trunc, output_type));
+      break;
+    }
 
     default:
       // TODO(nicohartmann): Support remaining operators.

src/compiler/simplified-lowering-verifier.h

@@ -16,26 +16,33 @@ class OperationTyper;
 class SimplifiedLoweringVerifier final {
  public:
   struct PerNodeData {
+    Type type = Type::None();
     Truncation truncation = Truncation::Any(IdentifyZeros::kDistinguishZeros);
   };
 
   SimplifiedLoweringVerifier(Zone* zone, Graph* graph)
-      : type_guards_(zone), data_(zone), graph_(graph) {}
+      : hints_(zone), data_(zone), graph_(graph) {}
 
   void VisitNode(Node* node, OperationTyper& op_typer);
 
-  void RecordTypeGuard(Node* node) {
-    DCHECK_EQ(node->opcode(), IrOpcode::kTypeGuard);
-    DCHECK(!is_recorded_type_guard(node));
-    type_guards_.insert(node);
-  }
-  const ZoneUnorderedSet<Node*>& recorded_type_guards() const {
-    return type_guards_;
+  void RecordHint(Node* node) {
+    DCHECK_EQ(node->opcode(), IrOpcode::kSLVerifierHint);
+    hints_.push_back(node);
   }
+  const ZoneVector<Node*>& inserted_hints() const { return hints_; }
 
  private:
-  bool is_recorded_type_guard(Node* node) const {
-    return type_guards_.find(node) != type_guards_.end();
+  void ResizeDataIfNecessary(Node* node) {
+    if (data_.size() <= node->id()) {
+      data_.resize(node->id() + 1);
+    }
+    DCHECK_EQ(data_[node->id()].truncation,
+              Truncation::Any(IdentifyZeros::kDistinguishZeros));
+  }
+
+  void SetType(Node* node, const Type& type) {
+    ResizeDataIfNecessary(node);
+    data_[node->id()].type = type;
   }
 
   Type InputType(Node* node, int input_index) const {
@@ -45,15 +52,16 @@ class SimplifiedLoweringVerifier final {
     if (NodeProperties::IsTyped(input)) {
       return NodeProperties::GetType(input);
     }
+    // For nodes that have not been typed before SL, we use the type that has
+    // been inferred by the verifier.
+    if (input->id() < data_.size()) {
+      return data_[input->id()].type;
+    }
     return Type::None();
   }
 
   void SetTruncation(Node* node, const Truncation& truncation) {
-    if (data_.size() <= node->id()) {
-      data_.resize(node->id() + 1);
-    }
-    DCHECK_EQ(data_[node->id()].truncation,
-              Truncation::Any(IdentifyZeros::kDistinguishZeros));
+    ResizeDataIfNecessary(node);
     data_[node->id()].truncation = truncation;
   }
 
@@ -68,6 +76,7 @@ class SimplifiedLoweringVerifier final {
     return any_truncation;
   }
 
+  void CheckType(Node* node, const Type& type);
   void CheckAndSet(Node* node, const Type& type, const Truncation& trunc);
 
   // Generalize to a less strict truncation in the context of a given type. For
@@ -81,7 +90,7 @@ class SimplifiedLoweringVerifier final {
 
   Zone* graph_zone() const { return graph_->zone(); }
 
-  ZoneUnorderedSet<Node*> type_guards_;
+  ZoneVector<Node*> hints_;
   ZoneVector<PerNodeData> data_;
   Graph* graph_;
 };

src/compiler/simplified-lowering.cc

@@ -741,12 +741,9 @@ class RepresentationSelector {
     // Verify all nodes.
     for (Node* node : traversal_nodes_) verifier_->VisitNode(node, op_typer_);
 
-    // Eliminate all introduced TypeGuard nodes.
-    for (Node* node : verifier_->recorded_type_guards()) {
+    // Eliminate all introduced hints.
+    for (Node* node : verifier_->inserted_hints()) {
       Node* input = node->InputAt(0);
-      DCHECK_EQ(node->InputAt(1), graph()->start());
-      DCHECK_EQ(node->InputAt(2), graph()->start());
-      DisconnectFromEffectAndControl(node);
       node->ReplaceUses(input);
       node->Kill();
     }
@@ -2106,7 +2103,7 @@ class RepresentationSelector {
           VisitLeaf<T>(node, MachineRepresentation::kTaggedSigned);
           if (lower<T>()) {
             intptr_t smi = bit_cast<intptr_t>(Smi::FromInt(value_as_int));
-            Node* constant = InsertTypeGuardForVerifier(
+            Node* constant = InsertTypeOverrideForVerifier(
                 NodeProperties::GetType(node),
                 lowering->jsgraph()->IntPtrConstant(smi));
             DeferReplacement(node, constant);
@@ -3549,7 +3546,9 @@ class RepresentationSelector {
       case IrOpcode::kPlainPrimitiveToNumber: {
         if (InputIs(node, Type::Boolean())) {
           VisitUnop<T>(node, UseInfo::Bool(), MachineRepresentation::kWord32);
-          if (lower<T>()) DeferReplacement(node, node->InputAt(0));
+          if (lower<T>()) {
+            ChangeToSemanticsHintForVerifier(node, node->op());
+          }
         } else if (InputIs(node, Type::String())) {
           VisitUnop<T>(node, UseInfo::AnyTagged(),
                        MachineRepresentation::kTagged);
@@ -3560,7 +3559,9 @@ class RepresentationSelector {
           if (InputIs(node, Type::NumberOrOddball())) {
             VisitUnop<T>(node, UseInfo::TruncatingWord32(),
                          MachineRepresentation::kWord32);
-            if (lower<T>()) DeferReplacement(node, node->InputAt(0));
+            if (lower<T>()) {
+              ChangeToSemanticsHintForVerifier(node, node->op());
+            }
           } else {
             VisitUnop<T>(node, UseInfo::AnyTagged(),
                          MachineRepresentation::kWord32);
@@ -3572,7 +3573,9 @@ class RepresentationSelector {
           if (InputIs(node, Type::NumberOrOddball())) {
             VisitUnop<T>(node, UseInfo::TruncatingFloat64(),
                          MachineRepresentation::kFloat64);
-            if (lower<T>()) DeferReplacement(node, node->InputAt(0));
+            if (lower<T>()) {
+              ChangeToSemanticsHintForVerifier(node, node->op());
+            }
           } else {
             VisitUnop<T>(node, UseInfo::AnyTagged(),
                          MachineRepresentation::kFloat64);
@@ -4086,16 +4089,27 @@ class RepresentationSelector {
     NotifyNodeReplaced(node, replacement);
   }
 
-  Node* InsertTypeGuardForVerifier(const Type& type, Node* node) {
+  Node* InsertTypeOverrideForVerifier(const Type& type, Node* node) {
     if (verification_enabled()) {
       DCHECK(!type.IsInvalid());
-      node = graph()->NewNode(common()->TypeGuard(type), node, graph()->start(),
-                              graph()->start());
-      verifier_->RecordTypeGuard(node);
+      node = graph()->NewNode(common()->SLVerifierHint(nullptr, type), node);
+      verifier_->RecordHint(node);
     }
     return node;
   }
 
+  void ChangeToSemanticsHintForVerifier(Node* node, const Operator* semantics) {
+    DCHECK_EQ(node->op()->ValueInputCount(), 1);
+    DCHECK_EQ(node->op()->EffectInputCount(), 0);
+    DCHECK_EQ(node->op()->ControlInputCount(), 0);
+    if (verification_enabled()) {
+      ChangeOp(node, common()->SLVerifierHint(semantics, base::nullopt));
+      verifier_->RecordHint(node);
+    } else {
+      DeferReplacement(node, node->InputAt(0));
+    }
+  }
+
  private:
   void ChangeOp(Node* node, const Operator* new_op) {
     compiler::NodeProperties::ChangeOp(node, new_op);

src/compiler/typer.cc

@@ -1031,6 +1031,7 @@ Type Typer::Visitor::TypeUnreachable(Node* node) { return Type::None(); }
 
 Type Typer::Visitor::TypePlug(Node* node) { UNREACHABLE(); }
 Type Typer::Visitor::TypeStaticAssert(Node* node) { UNREACHABLE(); }
+Type Typer::Visitor::TypeSLVerifierHint(Node* node) { UNREACHABLE(); }
 
 // JS comparison operators.
 

src/compiler/verifier.cc

@@ -1628,6 +1628,10 @@ void Verifier::Visitor::Check(Node* node, const AllNodes& all) {
       CHECK_GE(value_count, 1);
       CheckValueInputIs(node, 0, Type::Any());  // receiver
       break;
+    case IrOpcode::kSLVerifierHint:
+      // SLVerifierHint is internal to SimplifiedLowering and should never be
+      // seen by the verifier.
+      UNREACHABLE();
 #if V8_ENABLE_WEBASSEMBLY
     case IrOpcode::kJSWasmCall:
       CHECK_GE(value_count, 3);

src/flags/flag-definitions.h

@@ -569,7 +569,8 @@ DEFINE_BOOL(assert_types, false,
 // TODO(tebbi): Support allocating types from background thread.
 DEFINE_NEG_IMPLICATION(assert_types, concurrent_recompilation)
 
-DEFINE_BOOL(verify_simplified_lowering, false,
+// Enable verification of SimplifiedLowering in debug builds.
+DEFINE_BOOL(verify_simplified_lowering, DEBUG_BOOL,
             "verify graph generated by simplified lowering")
 
 DEFINE_BOOL(trace_compilation_dependencies, false, "trace code dependencies")

test/mjsunit/compiler/regress-1302572.js

+// Copyright 2022 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --allow-natives-syntax
+
+function foo(i) {
+  const b = i <= i;
+  return 0 + b;
+}
+
+%PrepareFunctionForOptimization(foo);
+assertEquals(1, foo(5));
+%OptimizeFunctionOnNextCall(foo);
+assertEquals(1, foo(5));