Skip to content

Commit ba44420

Browse files
committed
[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
1 parent 03339ac commit ba44420

File tree

2 files changed

+54
-5
lines changed

2 files changed

+54
-5
lines changed
Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
class Queue {
2+
_head;
3+
_tail;
4+
_length;
5+
constructor(items) {
6+
this._head = null;
7+
this._tail = null;
8+
this._length = 0;
9+
if (items) {
10+
for (const item of items) {
11+
this.enqueue(item);
12+
}
13+
}
14+
}
15+
enqueue(item) {
16+
const entry = {
17+
next: null,
18+
value: item,
19+
};
20+
if (this._tail) {
21+
this._tail.next = entry;
22+
this._tail = entry;
23+
} else {
24+
this._head = entry;
25+
this._tail = entry;
26+
}
27+
this._length++;
28+
}
29+
dequeue() {
30+
const entry = this._head;
31+
if (entry) {
32+
this._head = entry.next;
33+
this._length--;
34+
if (this._head === null) {
35+
this._tail = null;
36+
}
37+
return entry.value;
38+
} else {
39+
return null;
40+
}
41+
}
42+
}
43+
for (let i = 0; i < 1e5; i++) {
44+
const queue = new Queue(new Set(["foo", "bar", "baz"]));
45+
if (queue.dequeue() !== "foo") {
46+
throw new Error("Expected foo");
47+
}
48+
if (queue.dequeue() !== "bar") {
49+
throw new Error("Expected bar");
50+
}
51+
if (queue.dequeue() !== "baz") {
52+
throw new Error("Expected baz");
53+
}
54+
}

Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1909,11 +1909,6 @@ class ObjectAllocationSinkingPhase : public Phase {
19091909
if (m_heapAtHead[block].getAllocation(location.base()).isEscapedAllocation())
19101910
return nullptr;
19111911

1912-
// If we point to a single allocation, we will
1913-
// directly use its materialization
1914-
if (m_heapAtHead[block].follow(location))
1915-
return nullptr;
1916-
19171912
Node* phiNode = m_graph.addNode(SpecHeapTop, Phi, block->at(0)->origin.withInvalidExit());
19181913
phiNode->mergeFlags(NodeResultJS);
19191914
return phiNode;

0 commit comments

Comments
 (0)