From b7761100e367af60411e1f0cb1519d815d1959ce Mon Sep 17 00:00:00 2001
From: leszeks <leszeks@chromium.org>
Date: Fri, 4 Nov 2016 03:40:18 -0700
Subject: [PATCH] [turbofan] Do not replace actual duplicates when value
 numbering

The value numbering reducer has collision checks for nodes that mutated,
but kept the same hash, and are now equivalent to another node. However,
it can also accidentally hit itself, if it mutated, changed hash, but
ran over the location it was previously in.

After this patch, it checks to see if it is comparing against itself,
and skips over itself. Additionally, if this check is at the end of the
collisions, it opportunistically removes the duplicate entry and reduces
the size pressure on the list. We can do the same opportunistic clean up
if we happen to find a colliding equivalent entry, since we move it from
its original position to a new one.

Drive-by change: Ensure that the collision replacement checks types in
the same way that normal replacement does.

Review-Url: https://codereview.chromium.org/2475653002
Cr-Commit-Position: refs/heads/master@{#40757}
---
 src/compiler/value-numbering-reducer.cc | 71 +++++++++++++++++--------
 src/compiler/value-numbering-reducer.h  |  1 +
 2 files changed, 49 insertions(+), 23 deletions(-)

diff --git a/src/compiler/value-numbering-reducer.cc b/src/compiler/value-numbering-reducer.cc
index b9161df6a2..30473f2798 100644
--- a/src/compiler/value-numbering-reducer.cc
+++ b/src/compiler/value-numbering-reducer.cc
@@ -112,10 +112,31 @@ Reduction ValueNumberingReducer::Reduce(Node* node) {
         if (entry->IsDead()) {
           continue;
         }
+        if (entry == node) {
+          // Collision with ourselves, doesn't count as a real collision.
+          // Opportunistically clean-up the duplicate entry if we're at the end
+          // of a bucket.
+          if (!entries_[(j + 1) & mask]) {
+            entries_[j] = nullptr;
+            size_--;
+            return NoChange();
+          }
+          // Otherwise, keep searching for another collision.
+          continue;
+        }
         if (Equals(entry, node)) {
-          // Overwrite the colliding entry with the actual entry.
-          entries_[i] = entry;
-          return Replace(entry);
+          Reduction reduction = ReplaceIfTypesMatch(node, entry);
+          if (reduction.Changed()) {
+            // Overwrite the colliding entry with the actual entry.
+            entries_[i] = entry;
+            // Opportunistically clean-up the duplicate entry if we're at the
+            // end of a bucket.
+            if (!entries_[(j + 1) & mask]) {
+              entries_[j] = nullptr;
+              size_--;
+            }
+          }
+          return reduction;
         }
       }
     }
@@ -126,30 +147,34 @@ Reduction ValueNumberingReducer::Reduce(Node* node) {
       continue;
     }
     if (Equals(entry, node)) {
-      // Make sure the replacement has at least as good type as the original
-      // node.
-      if (NodeProperties::IsTyped(entry) && NodeProperties::IsTyped(node)) {
-        Type* entry_type = NodeProperties::GetType(entry);
-        Type* node_type = NodeProperties::GetType(node);
-        if (!entry_type->Is(node_type)) {
-          // Ideally, we would set an intersection of {entry_type} and
-          // {node_type} here. However, typing of NumberConstants assigns
-          // different types to constants with the same value (it creates
-          // a fresh heap number), which would make the intersection empty.
-          // To be safe, we use the smaller type if the types are comparable.
-          if (node_type->Is(entry_type)) {
-            NodeProperties::SetType(entry, node_type);
-          } else {
-            // Types are not comparable => do not replace.
-            return NoChange();
-          }
-        }
-      }
-      return Replace(entry);
+      return ReplaceIfTypesMatch(node, entry);
     }
   }
 }
 
+Reduction ValueNumberingReducer::ReplaceIfTypesMatch(Node* node,
+                                                     Node* replacement) {
+  // Make sure the replacement has at least as good type as the original node.
+  if (NodeProperties::IsTyped(replacement) && NodeProperties::IsTyped(node)) {
+    Type* replacement_type = NodeProperties::GetType(replacement);
+    Type* node_type = NodeProperties::GetType(node);
+    if (!replacement_type->Is(node_type)) {
+      // Ideally, we would set an intersection of {replacement_type} and
+      // {node_type} here. However, typing of NumberConstants assigns different
+      // types to constants with the same value (it creates a fresh heap
+      // number), which would make the intersection empty. To be safe, we use
+      // the smaller type if the types are comparable.
+      if (node_type->Is(replacement_type)) {
+        NodeProperties::SetType(replacement, node_type);
+      } else {
+        // Types are not comparable => do not replace.
+        return NoChange();
+      }
+    }
+  }
+  return Replace(replacement);
+}
+
 
 void ValueNumberingReducer::Grow() {
   // Allocate a new block of entries double the previous capacity.
diff --git a/src/compiler/value-numbering-reducer.h b/src/compiler/value-numbering-reducer.h
index f52e13b45c..521ce59f20 100644
--- a/src/compiler/value-numbering-reducer.h
+++ b/src/compiler/value-numbering-reducer.h
@@ -24,6 +24,7 @@ class V8_EXPORT_PRIVATE ValueNumberingReducer final
  private:
   enum { kInitialCapacity = 256u };
 
+  Reduction ReplaceIfTypesMatch(Node* node, Node* replacement);
   void Grow();
   Zone* temp_zone() const { return temp_zone_; }
   Zone* graph_zone() const { return graph_zone_; }