From ba44420c913e8089e187ae42b9fe596f0332fd05 Mon Sep 17 00:00:00 2001
From: Yusuke Suzuki <ysuzuki@apple.com>
Date: Thu, 12 Sep 2024 09:33:10 -0700
Subject: [PATCH] [JSC] ObjectAllocationSinking should not omit phi insertion
 when pointer follows to the same value
 https://bugs.webkit.org/show_bug.cgi?id=279570 rdar://135851156

Reviewed by Keith Miller.

Let's consider the following FTL graph.

    BB#0
    @0 = NewObject()
    Jump #1

    BB#1
    PutByOffset(@0, 0, @x)
    Jump #2

    BB#2
    ...
    @z = ...
    @1 = GetByOffset(@x, 0)
    Branch(@1, #3, #4)

    BB#3
    PutByOffset(@0, 0, @z)
    Jump #5

    BB#4
    PutByOffset(@0, 0, @z)
    Jump #5

    BB#5
    Jump #2

Now, we would like to eliminate @0 object allocation. And we are
computing SSA for pointers of fields of the that object which gets
eliminated. Consider about @x's fields' SSA. PutByOffset becomes Def
and GetByOffset becomes Use. And the same field will get the same SSA
variable. So we first puts Defs and compute Phis based on that.

In ObjectAllocationSinking phase, we had a fast path when the both SSA
variable is following to the same value. Let's see BB#5. Because BB#3
and BB#4 defines Defs, dominance frontier BB#5 will need to introduce
Phi. But interestingly, both SSA variable is following to the same @z.
As a result, we were not inserting Phi for this case.

But this is wrong. Inserted Phi is a Def, and based on that, we will
further introduce Phis with that. If we omit inserting Phi in BB#5,
we will not insert Phi into BB#2 while BB#2 will merge BB#1's Def And
BB#5's Phi's Def. As a result, in BB#2, we think this variable is
following to BB#1's Def. But that's wrong and BB#5's Phi exists.

This patch removes this fast path to fix the issue.

* JSTests/stress/object-allocation-sinking-phi-insertion-for-pointers.js: Added.
(Queue):
(Queue.prototype.enqueue):
(Queue.prototype.dequeue):
(i.queue.dequeue):
* Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp:

Canonical link: https://commits.webkit.org/283558@main
---
 ...tion-sinking-phi-insertion-for-pointers.js | 54 +++++++++++++++++++
 .../dfg/DFGObjectAllocationSinkingPhase.cpp   |  5 --
 2 files changed, 54 insertions(+), 5 deletions(-)
 create mode 100644 JSTests/stress/object-allocation-sinking-phi-insertion-for-pointers.js

diff --git a/JSTests/stress/object-allocation-sinking-phi-insertion-for-pointers.js b/JSTests/stress/object-allocation-sinking-phi-insertion-for-pointers.js
new file mode 100644
index 0000000000000..d940ed42d4010
--- /dev/null
+++ b/JSTests/stress/object-allocation-sinking-phi-insertion-for-pointers.js
@@ -0,0 +1,54 @@
+class Queue {
+  _head;
+  _tail;
+  _length;
+  constructor(items) {
+    this._head = null;
+    this._tail = null;
+    this._length = 0;
+    if (items) {
+      for (const item of items) {
+        this.enqueue(item);
+      }
+    }
+  }
+  enqueue(item) {
+    const entry = {
+      next: null,
+      value: item,
+    };
+    if (this._tail) {
+      this._tail.next = entry;
+      this._tail = entry;
+    } else {
+      this._head = entry;
+      this._tail = entry;
+    }
+    this._length++;
+  }
+  dequeue() {
+    const entry = this._head;
+    if (entry) {
+      this._head = entry.next;
+      this._length--;
+      if (this._head === null) {
+        this._tail = null;
+      }
+      return entry.value;
+    } else {
+      return null;
+    }
+  }
+}
+for (let i = 0; i < 1e5; i++) {
+  const queue = new Queue(new Set(["foo", "bar", "baz"]));
+  if (queue.dequeue() !== "foo") {
+    throw new Error("Expected foo");
+  }
+  if (queue.dequeue() !== "bar") {
+    throw new Error("Expected bar");
+  }
+  if (queue.dequeue() !== "baz") {
+    throw new Error("Expected baz");
+  }
+}
diff --git a/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp b/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
index 60092acf69e5a..1e400a2787d9d 100644
--- a/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
+++ b/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
@@ -1909,11 +1909,6 @@ class ObjectAllocationSinkingPhase : public Phase {
                 if (m_heapAtHead[block].getAllocation(location.base()).isEscapedAllocation())
                     return nullptr;
 
-                // If we point to a single allocation, we will
-                // directly use its materialization
-                if (m_heapAtHead[block].follow(location))
-                    return nullptr;
-
                 Node* phiNode = m_graph.addNode(SpecHeapTop, Phi, block->at(0)->origin.withInvalidExit());
                 phiNode->mergeFlags(NodeResultJS);
                 return phiNode;