[maglev] Use known_node_aspects for representation changes

Since we have the NodeInfo map in known_node_aspects now anyway, use it
for representation changes instead of the current interpreter frame
value swapping mechanism.

We originally didn't want a map for representations, but if we have one
anyway for node types, it makes sense to merge the two mechanisms.

This also allows us to get rid of the "register same as accumulator"
hack for binops, which was trying to avoid redundant representation
changes, since the redundancy is removed by the node aspects mechanism
automatically. It's kept for CheckedInternalizedString, since that isn't
a representation change within our framework (maybe it should be?).

Bug: v8:7700
Change-Id: I92491307f83d3b63fc7a210f6b8754e45d3531a7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3905182Reviewed-by: Jakob Linke <jgruber@chromium.org>
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/main@{#83317}

src/maglev/maglev-graph-builder.cc

@@ -327,13 +327,9 @@ void MaglevGraphBuilder::BuildGenericBinarySmiOperationNode() {
 template <Operation kOperation>
 void MaglevGraphBuilder::BuildInt32BinaryOperationNode() {
   // TODO(v8:7700): Do constant folding.
-  ValueNode *left, *right;
-  if (IsRegisterEqualToAccumulator(0)) {
-    left = right = LoadRegisterInt32(0);
-  } else {
-    left = LoadRegisterInt32(0);
-    right = GetAccumulatorInt32();
-  }
+  ValueNode* left = LoadRegisterInt32(0);
+  ValueNode* right = GetAccumulatorInt32();
+
   SetAccumulator(AddNewInt32BinaryOperationNode<kOperation>({left, right}));
 }
 
@@ -363,13 +359,9 @@ void MaglevGraphBuilder::BuildFloat64BinarySmiOperationNode() {
 template <Operation kOperation>
 void MaglevGraphBuilder::BuildFloat64BinaryOperationNode() {
   // TODO(v8:7700): Do constant folding.
-  ValueNode *left, *right;
-  if (IsRegisterEqualToAccumulator(0)) {
-    left = right = LoadRegisterFloat64(0);
-  } else {
-    left = LoadRegisterFloat64(0);
-    right = GetAccumulatorFloat64();
-  }
+  ValueNode* left = LoadRegisterFloat64(0);
+  ValueNode* right = GetAccumulatorFloat64();
+
   SetAccumulator(AddNewFloat64BinaryOperationNode<kOperation>({left, right}));
 }
 
@@ -505,13 +497,9 @@ void MaglevGraphBuilder::VisitCompareOperation() {
       return;
     case CompareOperationHint::kSignedSmall:
       if (BinaryOperationHasInt32FastPath<kOperation>()) {
-        ValueNode *left, *right;
-        if (IsRegisterEqualToAccumulator(0)) {
-          left = right = LoadRegisterInt32(0);
-        } else {
-          left = LoadRegisterInt32(0);
-          right = GetAccumulatorInt32();
-        }
+        ValueNode* left = LoadRegisterInt32(0);
+        ValueNode* right = GetAccumulatorInt32();
+
         if (TryBuildCompareOperation<BranchIfInt32Compare>(kOperation, left,
                                                            right)) {
           return;
@@ -523,13 +511,9 @@ void MaglevGraphBuilder::VisitCompareOperation() {
       break;
     case CompareOperationHint::kNumber:
       if (BinaryOperationHasFloat64FastPath<kOperation>()) {
-        ValueNode *left, *right;
-        if (IsRegisterEqualToAccumulator(0)) {
-          left = right = LoadRegisterFloat64(0);
-        } else {
-          left = LoadRegisterFloat64(0);
-          right = GetAccumulatorFloat64();
-        }
+        ValueNode* left = LoadRegisterFloat64(0);
+        ValueNode* right = GetAccumulatorFloat64();
+
         if (TryBuildCompareOperation<BranchIfFloat64Compare>(kOperation, left,
                                                              right)) {
           return;
@@ -550,10 +534,28 @@ void MaglevGraphBuilder::VisitCompareOperation() {
              kOperation == Operation::kStrictEqual);
       ValueNode *left, *right;
       if (IsRegisterEqualToAccumulator(0)) {
-        left = right = LoadRegister<CheckedInternalizedString>(0);
+        interpreter::Register reg = iterator_.GetRegisterOperand(0);
+        ValueNode* value = GetTaggedValue(reg);
+        if (!value->Is<CheckedInternalizedString>()) {
+          value = AddNewNode<CheckedInternalizedString>({value});
+          current_interpreter_frame_.set(reg, value);
+          current_interpreter_frame_.set(
+              interpreter::Register::virtual_accumulator(), value);
+        }
+        left = right = value;
       } else {
-        left = LoadRegister<CheckedInternalizedString>(0);
-        right = GetAccumulator<CheckedInternalizedString>();
+        interpreter::Register reg = iterator_.GetRegisterOperand(0);
+        left = GetTaggedValue(reg);
+        if (!left->Is<CheckedInternalizedString>()) {
+          left = AddNewNode<CheckedInternalizedString>({left});
+          current_interpreter_frame_.set(reg, left);
+        }
+        right = GetAccumulatorTagged();
+        if (!right->Is<CheckedInternalizedString>()) {
+          right = AddNewNode<CheckedInternalizedString>({right});
+          current_interpreter_frame_.set(
+              interpreter::Register::virtual_accumulator(), right);
+        }
       }
       if (TryBuildCompareOperation<BranchIfReferenceCompare>(kOperation, left,
                                                              right)) {
@@ -565,16 +567,10 @@ void MaglevGraphBuilder::VisitCompareOperation() {
     case CompareOperationHint::kSymbol: {
       DCHECK(kOperation == Operation::kEqual ||
              kOperation == Operation::kStrictEqual);
-      ValueNode *left, *right;
-      if (IsRegisterEqualToAccumulator(0)) {
-        left = right = LoadRegisterTagged(0);
-        BuildCheckSymbol(left);
-      } else {
-        left = LoadRegisterTagged(0);
-        right = GetAccumulatorTagged();
-        BuildCheckSymbol(left);
-        BuildCheckSymbol(right);
-      }
+      ValueNode* left = LoadRegisterTagged(0);
+      ValueNode* right = GetAccumulatorTagged();
+      BuildCheckSymbol(left);
+      BuildCheckSymbol(right);
       if (TryBuildCompareOperation<BranchIfReferenceCompare>(kOperation, left,
                                                              right)) {
         return;
@@ -920,27 +916,25 @@ void MaglevGraphBuilder::VisitStaLookupSlot() {
 }
 
 void MaglevGraphBuilder::BuildCheckSmi(ValueNode* object) {
-  NodeInfo* known_info = known_node_aspects().GetInfoFor(object);
-  if (NodeInfo::IsSmi(known_info)) return;
+  NodeInfo* known_info = known_node_aspects().GetOrCreateInfoFor(object);
+  if (known_info->is_smi()) return;
 
   // TODO(leszeks): Figure out a way to also handle CheckedSmiUntag.
   AddNewNode<CheckSmi>({object});
-  known_node_aspects().InsertOrUpdateNodeType(object, known_info,
-                                              NodeType::kSmi);
+  known_info->type = NodeType::kSmi;
 }
 
 void MaglevGraphBuilder::BuildCheckHeapObject(ValueNode* object) {
-  NodeInfo* known_info = known_node_aspects().GetInfoFor(object);
-  if (NodeInfo::IsAnyHeapObject(known_info)) return;
+  NodeInfo* known_info = known_node_aspects().GetOrCreateInfoFor(object);
+  if (known_info->is_any_heap_object()) return;
 
   AddNewNode<CheckHeapObject>({object});
-  known_node_aspects().InsertOrUpdateNodeType(object, known_info,
-                                              NodeType::kAnyHeapObject);
+  known_info->type = NodeType::kAnyHeapObject;
 }
 
 namespace {
 CheckType GetCheckType(NodeInfo* known_info) {
-  if (NodeInfo::IsAnyHeapObject(known_info)) {
+  if (known_info->is_any_heap_object()) {
     return CheckType::kOmitHeapObjectCheck;
   }
   return CheckType::kCheckHeapObject;
@@ -948,21 +942,19 @@ CheckType GetCheckType(NodeInfo* known_info) {
 }  // namespace
 
 void MaglevGraphBuilder::BuildCheckString(ValueNode* object) {
-  NodeInfo* known_info = known_node_aspects().GetInfoFor(object);
-  if (NodeInfo::IsString(known_info)) return;
+  NodeInfo* known_info = known_node_aspects().GetOrCreateInfoFor(object);
+  if (known_info->is_string()) return;
 
   AddNewNode<CheckString>({object}, GetCheckType(known_info));
-  known_node_aspects().InsertOrUpdateNodeType(object, known_info,
-                                              NodeType::kString);
+  known_info->type = NodeType::kString;
 }
 
 void MaglevGraphBuilder::BuildCheckSymbol(ValueNode* object) {
-  NodeInfo* known_info = known_node_aspects().GetInfoFor(object);
-  if (NodeInfo::IsSymbol(known_info)) return;
+  NodeInfo* known_info = known_node_aspects().GetOrCreateInfoFor(object);
+  if (known_info->is_symbol()) return;
 
   AddNewNode<CheckSymbol>({object}, GetCheckType(known_info));
-  known_node_aspects().InsertOrUpdateNodeType(object, known_info,
-                                              NodeType::kSymbol);
+  known_info->type = NodeType::kSymbol;
 }
 
 void MaglevGraphBuilder::BuildMapCheck(ValueNode* object,
@@ -979,7 +971,7 @@ void MaglevGraphBuilder::BuildMapCheck(ValueNode* object,
     // TODO(leszeks): Insert an unconditional deopt if the known type doesn't
     // match the required type.
   }
-  NodeInfo* known_info = known_node_aspects().GetInfoFor(object);
+  NodeInfo* known_info = known_node_aspects().GetOrCreateInfoFor(object);
   if (map.is_migration_target()) {
     AddNewNode<CheckMapsWithMigration>({object}, map, GetCheckType(known_info));
   } else {
@@ -989,8 +981,7 @@ void MaglevGraphBuilder::BuildMapCheck(ValueNode* object,
   if (map.is_stable()) {
     compilation_unit_->broker()->dependencies()->DependOnStableMap(map);
   }
-  known_node_aspects().InsertOrUpdateNodeType(
-      object, known_info, NodeType::kHeapObjectWithKnownMap);
+  known_info->type = NodeType::kHeapObjectWithKnownMap;
 }
 
 bool MaglevGraphBuilder::TryBuildMonomorphicLoad(ValueNode* receiver,
@@ -1181,15 +1172,23 @@ bool MaglevGraphBuilder::TryBuildMonomorphicElementLoadFromSmiHandler(
       BuildMapCheck(object, map);
       switch (index->properties().value_representation()) {
         case ValueRepresentation::kTagged: {
-          if (CheckedSmiTag* checked_untag = index->TryCast<CheckedSmiTag>()) {
-            index = checked_untag->input().node();
-          } else if (SmiConstant* constant = index->TryCast<SmiConstant>()) {
+          if (SmiConstant* constant = index->TryCast<SmiConstant>()) {
             index = GetInt32Constant(constant->value().value());
-          } else if (NodeInfo::IsSmi(known_node_aspects().GetInfoFor(index))) {
-            // TODO(leszeks): This could be unchecked.
-            index = AddNewNode<CheckedSmiUntag>({index});
           } else {
-            index = AddNewNode<CheckedObjectToIndex>({index});
+            NodeInfo* node_info =
+                known_node_aspects().GetOrCreateInfoFor(index);
+            if (node_info->is_smi()) {
+              if (!node_info->int32_alternative) {
+                // TODO(leszeks): This could be unchecked.
+                node_info->int32_alternative =
+                    AddNewNode<CheckedSmiUntag>({index});
+              }
+              index = node_info->int32_alternative;
+            } else {
+              // TODO(leszeks): Cache this knowledge/converted value somehow on
+              // the node info.
+              index = AddNewNode<CheckedObjectToIndex>({index});
+            }
           }
           break;
         }

src/maglev/maglev-graph-builder.h

@@ -621,76 +621,52 @@ class MaglevGraphBuilder {
     current_interpreter_frame_.set(dst, current_interpreter_frame_.get(src));
   }
 
-  template <typename NodeT>
-  ValueNode* AddNewConversionNode(interpreter::Register reg, ValueNode* node) {
-    // TODO(v8:7700): Use a canonical conversion node. Maybe like in Phi nodes
-    // where we always add a the conversion immediately after the ValueNode.
-    DCHECK(NodeT::kProperties.is_conversion());
-    ValueNode* result = AddNewNode<NodeT>({node});
-    current_interpreter_frame_.set(reg, result);
-    return result;
-  }
-
-  ValueNode* GetTaggedValueHelper(interpreter::Register reg, ValueNode* value) {
-    // TODO(victorgomes): Consider adding the representation in the
-    // InterpreterFrameState, so that we don't need to derefence a node.
+  ValueNode* GetTaggedValue(interpreter::Register reg) {
+    ValueNode* value = current_interpreter_frame_.get(reg);
     switch (value->properties().value_representation()) {
       case ValueRepresentation::kTagged:
         return value;
       case ValueRepresentation::kInt32: {
-        if (value->Is<CheckedSmiUntag>()) {
-          return value->input(0).node();
+        NodeInfo* node_info = known_node_aspects().GetOrCreateInfoFor(value);
+        if (node_info->tagged_alternative == nullptr) {
+          node_info->tagged_alternative = AddNewNode<CheckedSmiTag>({value});
         }
-        return AddNewConversionNode<CheckedSmiTag>(reg, value);
+        return node_info->tagged_alternative;
       }
       case ValueRepresentation::kFloat64: {
-        if (value->Is<CheckedFloat64Unbox>()) {
-          return value->input(0).node();
-        }
-        if (value->Is<ChangeInt32ToFloat64>()) {
-          ValueNode* int32_value = value->input(0).node();
-          return GetTaggedValueHelper(reg, int32_value);
+        NodeInfo* node_info = known_node_aspects().GetOrCreateInfoFor(value);
+        if (node_info->tagged_alternative == nullptr) {
+          node_info->tagged_alternative = AddNewNode<Float64Box>({value});
         }
-        return AddNewConversionNode<Float64Box>(reg, value);
+        return node_info->tagged_alternative;
       }
     }
     UNREACHABLE();
   }
 
-  ValueNode* GetTaggedValue(interpreter::Register reg) {
-    ValueNode* value = current_interpreter_frame_.get(reg);
-    return GetTaggedValueHelper(reg, value);
-  }
-
-  template <typename ConversionNodeT>
-  ValueNode* GetValue(interpreter::Register reg) {
-    ValueNode* value = current_interpreter_frame_.get(reg);
-    return AddNewConversionNode<ConversionNodeT>(reg, value);
-  }
-
   ValueNode* GetInt32(interpreter::Register reg) {
     ValueNode* value = current_interpreter_frame_.get(reg);
     switch (value->properties().value_representation()) {
       case ValueRepresentation::kTagged: {
-        if (value->Is<CheckedSmiTag>()) {
-          return value->input(0).node();
-        } else if (SmiConstant* constant = value->TryCast<SmiConstant>()) {
+        if (SmiConstant* constant = value->TryCast<SmiConstant>()) {
           return GetInt32Constant(constant->value().value());
         }
-        return AddNewConversionNode<CheckedSmiUntag>(reg, value);
+        NodeInfo* node_info = known_node_aspects().GetOrCreateInfoFor(value);
+        if (node_info->int32_alternative == nullptr) {
+          node_info->int32_alternative = AddNewNode<CheckedSmiUntag>({value});
+        }
+        return node_info->int32_alternative;
       }
       case ValueRepresentation::kInt32:
         return value;
-      case ValueRepresentation::kFloat64:
-        if (value->Is<CheckedFloat64Unbox>()) {
-          // TODO(leszeks): Maybe convert the CheckedFloat64Unbox to
-          // ChangeInt32ToFloat64 with this CheckedSmiUntag as the input.
-          return AddNewConversionNode<CheckedSmiUntag>(reg,
-                                                       value->input(0).node());
-        } else if (value->Is<ChangeInt32ToFloat64>()) {
-          return value->input(0).node();
+      case ValueRepresentation::kFloat64: {
+        NodeInfo* node_info = known_node_aspects().GetOrCreateInfoFor(value);
+        if (node_info->int32_alternative == nullptr) {
+          node_info->int32_alternative =
+              AddNewNode<CheckedTruncateFloat64ToInt32>({value});
         }
-        return AddNewConversionNode<CheckedTruncateFloat64ToInt32>(reg, value);
+        return node_info->int32_alternative;
+      }
     }
     UNREACHABLE();
   }
@@ -699,25 +675,27 @@ class MaglevGraphBuilder {
     ValueNode* value = current_interpreter_frame_.get(reg);
     switch (value->properties().value_representation()) {
       case ValueRepresentation::kTagged: {
-        if (value->Is<Float64Box>()) {
-          return value->input(0).node();
+        NodeInfo* node_info = known_node_aspects().GetOrCreateInfoFor(value);
+        if (node_info->float64_alternative == nullptr) {
+          node_info->float64_alternative =
+              AddNewNode<CheckedFloat64Unbox>({value});
         }
-        return AddNewConversionNode<CheckedFloat64Unbox>(reg, value);
+        return node_info->float64_alternative;
+      }
+      case ValueRepresentation::kInt32: {
+        NodeInfo* node_info = known_node_aspects().GetOrCreateInfoFor(value);
+        if (node_info->float64_alternative == nullptr) {
+          node_info->float64_alternative =
+              AddNewNode<ChangeInt32ToFloat64>({value});
+        }
+        return node_info->float64_alternative;
       }
-      case ValueRepresentation::kInt32:
-        return AddNewConversionNode<ChangeInt32ToFloat64>(reg, value);
       case ValueRepresentation::kFloat64:
         return value;
     }
     UNREACHABLE();
   }
 
-  template <typename ConversionNodeT>
-  ValueNode* GetAccumulator() {
-    return GetValue<ConversionNodeT>(
-        interpreter::Register::virtual_accumulator());
-  }
-
   ValueNode* GetAccumulatorTagged() {
     return GetTaggedValue(interpreter::Register::virtual_accumulator());
   }
@@ -736,12 +714,6 @@ class MaglevGraphBuilder {
            current_interpreter_frame_.accumulator();
   }
 
-  template <typename ConversionNodeT>
-  ValueNode* LoadRegister(int operand_index) {
-    return GetValue<ConversionNodeT>(
-        iterator_.GetRegisterOperand(operand_index));
-  }
-
   ValueNode* LoadRegisterTagged(int operand_index) {
     return GetTaggedValue(iterator_.GetRegisterOperand(operand_index));
   }

src/maglev/maglev-interpreter-frame-state.h

@@ -73,33 +73,37 @@ enum class NodeType {
 };
 
 struct NodeInfo {
-  NodeType type;
-  // TODO(leszeks): Consider adding more info for nodes here, e.g. alternative
-  // representations or previously loaded fields.
+  NodeType type = NodeType::kUnknown;
 
-  static bool IsSmi(const NodeInfo* info) {
-    if (!info) return false;
-    return info->type == NodeType::kSmi;
-  }
-  static bool IsAnyHeapObject(const NodeInfo* info) {
-    if (!info) return false;
-    return static_cast<int>(info->type) &
-           static_cast<int>(NodeType::kAnyHeapObject);
-  }
-  static bool IsString(const NodeInfo* info) {
-    if (!info) return false;
-    return info->type == NodeType::kString;
-  }
-  static bool IsSymbol(const NodeInfo* info) {
-    if (!info) return false;
-    return info->type == NodeType::kSymbol;
+  // Optional alternative nodes with the equivalent value but a different
+  // representation.
+  // TODO(leszeks): At least one of these is redundant for every node, consider
+  // a more compressed form or even linked list.
+  ValueNode* tagged_alternative = nullptr;
+  ValueNode* int32_alternative = nullptr;
+  ValueNode* float64_alternative = nullptr;
+
+  bool is_smi() const { return type == NodeType::kSmi; }
+  bool is_any_heap_object() const {
+    return static_cast<int>(type) & static_cast<int>(NodeType::kAnyHeapObject);
   }
+  bool is_string() const { return type == NodeType::kString; }
+  bool is_symbol() const { return type == NodeType::kSymbol; }
 
   // Mutate this node info by merging in another node info, with the result
   // being a node info that is the subset of information valid in both inputs.
   void MergeWith(const NodeInfo& other) {
     type = static_cast<NodeType>(static_cast<int>(type) &
                                  static_cast<int>(other.type));
+    tagged_alternative = tagged_alternative == other.tagged_alternative
+                             ? tagged_alternative
+                             : nullptr;
+    int32_alternative = int32_alternative == other.int32_alternative
+                            ? int32_alternative
+                            : nullptr;
+    float64_alternative = float64_alternative == other.float64_alternative
+                              ? float64_alternative
+                              : nullptr;
   }
 };
 
@@ -131,22 +135,7 @@ struct KnownNodeAspects {
     return clone;
   }
 
-  NodeInfo* GetInfoFor(ValueNode* node) {
-    auto it = node_infos.find(node);
-    if (it == node_infos.end()) return nullptr;
-    return &it->second;
-  }
-
-  void InsertOrUpdateNodeType(ValueNode* node, NodeInfo* existing_info,
-                              NodeType new_type) {
-    if (existing_info == nullptr) {
-      DCHECK_EQ(node_infos.find(node), node_infos.end());
-      node_infos.emplace(node, NodeInfo{new_type});
-    } else {
-      DCHECK_EQ(&node_infos.find(node)->second, existing_info);
-      existing_info->type = new_type;
-    }
-  }
+  NodeInfo* GetOrCreateInfoFor(ValueNode* node) { return &node_infos[node]; }
 
   void Merge(const KnownNodeAspects& other) {
     DestructivelyIntersect(node_infos, other.node_infos,
@@ -186,6 +175,8 @@ class InterpreterFrameState {
                        const MergePointInterpreterFrameState& state);
 
   void set_accumulator(ValueNode* value) {
+    // Conversions should be stored in known_node_aspects/NodeInfo.
+    DCHECK(!value->properties().is_conversion());
     frame_[interpreter::Register::virtual_accumulator()] = value;
   }
   ValueNode* accumulator() const {
@@ -198,6 +189,8 @@ class InterpreterFrameState {
                        reg == interpreter::Register::function_closure() ||
                        reg == interpreter::Register::virtual_accumulator() ||
                        reg.ToParameterIndex() >= 0);
+    // Conversions should be stored in known_node_aspects/NodeInfo.
+    DCHECK(!value->properties().is_conversion());
     frame_[reg] = value;
   }
   ValueNode* get(interpreter::Register reg) const {

src/maglev/maglev-ir.h

@@ -2763,9 +2763,8 @@ class CheckedInternalizedString
     CHECK_EQ(properties().value_representation(), ValueRepresentation::kTagged);
   }
 
-  static constexpr OpProperties kProperties = OpProperties::EagerDeopt() |
-                                              OpProperties::TaggedValue() |
-                                              OpProperties::ConversionNode();
+  static constexpr OpProperties kProperties =
+      OpProperties::EagerDeopt() | OpProperties::TaggedValue();
 
   static constexpr int kObjectIndex = 0;
   Input& object_input() { return Node::input(kObjectIndex); }