Skip to content

Commit

Permalink
Cherry-pick 47e039f. rdar://115399657
Browse files Browse the repository at this point in the history
    clobberize needs to be more precise with the *ByOffset nodes
    https://bugs.webkit.org/show_bug.cgi?id=261544
    rdar://115399657

    Reviewed by Yusuke Suzuki and Mark Lam.

    CSE phase uses clobberize to figure out if it's safe to merge two operations that
    def the same HeapLocation. Since HeapLocation does not currently have a way to
    track the offset used by the various *ByOffset nodes it can get confused and
    think that two ByOffset instructions produce the same value even if they don't
    use the same offset. This patch solves this by adding a new field to HeapLocation,
    which takes the metadata associated with the corresponding *ByOffset node. If two
    *ByOffset operations don't share the same metadata then they cannot be CSEed.

    * Source/JavaScriptCore/dfg/DFGCSEPhase.cpp:
    * Source/JavaScriptCore/dfg/DFGClobberize.h:
    (JSC::DFG::clobberize):
    * Source/JavaScriptCore/dfg/DFGHeapLocation.h:
    (JSC::DFG::HeapLocation::HeapLocation):
    (JSC::DFG::HeapLocation::extraState const):
    (JSC::DFG::HeapLocation::hash const):

    Canonical link: https://commits.webkit.org/265870.558@safari-7616-branch

Identifier: 259548.873@safari-7615.3.12.11-branch
  • Loading branch information
kmiller68 authored and MyahCobbs committed Sep 15, 2023
1 parent 2a0187c commit 60b4ab1
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 8 deletions.
25 changes: 25 additions & 0 deletions JSTests/stress/getbyoffset-cse-consistency.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
let a = {};
a.foo = 1;
a.bar = 1.1;

let b = {};
b.baz = 3.3;
b.foo = 1;

function func(flag) {
let tmp = flag ? a : b;

let x;
for (let i = 0; i < 10; ++i) {
if (flag)
x = tmp.foo;
else
x = tmp.foo;
}
return x;
}

for (let i = 0; i < 1e5; ++i) {
if (func(i % 2) !== 1)
throw new Error();
}
38 changes: 38 additions & 0 deletions JSTests/stress/multigetbyoffset-cse-consistency.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
let a = {};
a.foo = 1;
a.bar = 1.1;
a.other = 4.4;

let b = {};
b.baz = 3.3;
b.foo = 1;
b.other = 5.4;

let c = {};
c.other = 3.4;
c.other2 = 9.1;
c.foo = 1;

function func(flag1, flag2) {
let tmp = a;
if (flag1) {
tmp = b;
if (flag2)
tmp = c;
}


let x;
for (let i = 0; i < 10; ++i) {
if (flag1)
x = tmp.foo;
else
x = tmp.foo;
}
return x;
}

for (let i = 0; i < 1e5; ++i) {
if (func(i % 2, i % 3) !== 1)
throw new Error();
}
6 changes: 6 additions & 0 deletions Source/JavaScriptCore/dfg/DFGCSEPhase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,12 @@ class GlobalCSEPhase : public Phase {

bool run()
{

if (DFGCSEPhaseInternal::verbose) {
dataLog("Graph before Global CSE:\n");
m_graph.dump();
}

ASSERT(m_graph.m_fixpointState == FixpointNotConverged);
ASSERT(m_graph.m_form == SSA);

Expand Down
8 changes: 4 additions & 4 deletions Source/JavaScriptCore/dfg/DFGClobberize.h
Original file line number Diff line number Diff line change
Expand Up @@ -1439,7 +1439,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
unsigned identifierNumber = node->storageAccessData().identifierNumber;
AbstractHeap heap(NamedProperties, identifierNumber);
read(heap);
def(HeapLocation(NamedPropertyLoc, heap, node->child2()), LazyNode(node));
def(HeapLocation(NamedPropertyLoc, heap, node->child2(), &node->storageAccessData()), LazyNode(node));
return;
}

Expand All @@ -1448,7 +1448,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
read(JSObject_butterfly);
AbstractHeap heap(NamedProperties, node->multiGetByOffsetData().identifierNumber);
read(heap);
def(HeapLocation(NamedPropertyLoc, heap, node->child1()), LazyNode(node));
def(HeapLocation(NamedPropertyLoc, heap, node->child1(), &node->multiGetByOffsetData()), LazyNode(node));
return;
}

Expand All @@ -1461,7 +1461,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
write(JSCell_structureID);
if (node->multiPutByOffsetData().reallocatesStorage())
write(JSObject_butterfly);
def(HeapLocation(NamedPropertyLoc, heap, node->child1()), LazyNode(node->child2().node()));
def(HeapLocation(NamedPropertyLoc, heap, node->child1(), &node->multiPutByOffsetData()), LazyNode(node->child2().node()));
return;
}

Expand All @@ -1483,7 +1483,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
unsigned identifierNumber = node->storageAccessData().identifierNumber;
AbstractHeap heap(NamedProperties, identifierNumber);
write(heap);
def(HeapLocation(NamedPropertyLoc, heap, node->child2()), LazyNode(node->child3().node()));
def(HeapLocation(NamedPropertyLoc, heap, node->child2(), &node->storageAccessData()), LazyNode(node->child3().node()));
return;
}

Expand Down
25 changes: 21 additions & 4 deletions Source/JavaScriptCore/dfg/DFGHeapLocation.h
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,20 @@ class HeapLocation {
HeapLocation(
LocationKind kind = InvalidLocationKind,
AbstractHeap heap = AbstractHeap(),
Node* base = nullptr, LazyNode index = LazyNode(), Node* descriptor = nullptr)
Node* base = nullptr,
LazyNode index = LazyNode(),
Node* descriptor = nullptr,
void* extraState = nullptr)
: m_kind(kind)
, m_heap(heap)
, m_base(base)
, m_index(index)
, m_descriptor(descriptor)
, m_extraState(extraState)
{
ASSERT((kind == InvalidLocationKind) == !heap);
ASSERT(!!m_heap || !m_base);
ASSERT(m_base || (!m_index && !m_descriptor));
ASSERT(m_base || (!m_index && !m_descriptor && !m_extraState));
}

HeapLocation(LocationKind kind, AbstractHeap heap, Node* base, Node* index, Node* descriptor = nullptr)
Expand All @@ -110,6 +114,11 @@ class HeapLocation {
: HeapLocation(kind, heap, base.node(), index.node(), descriptor.node())
{
}

HeapLocation(LocationKind kind, AbstractHeap heap, Edge base, void* extraState)
: HeapLocation(kind, heap, base.node(), nullptr, nullptr, extraState)
{
}

HeapLocation(WTF::HashTableDeletedValueType)
: m_kind(InvalidLocationKind)
Expand All @@ -126,10 +135,16 @@ class HeapLocation {
AbstractHeap heap() const { return m_heap; }
Node* base() const { return m_base; }
LazyNode index() const { return m_index; }
void* extraState() const { return m_extraState; }

unsigned hash() const
{
return m_kind + m_heap.hash() + m_index.hash() + static_cast<unsigned>(bitwise_cast<uintptr_t>(m_base)) + static_cast<unsigned>(bitwise_cast<uintptr_t>(m_descriptor));
return m_kind
+ m_heap.hash()
+ m_index.hash()
+ static_cast<unsigned>(bitwise_cast<uintptr_t>(m_base))
+ static_cast<unsigned>(bitwise_cast<uintptr_t>(m_descriptor))
+ static_cast<unsigned>(bitwise_cast<uintptr_t>(m_extraState));
}

bool operator==(const HeapLocation& other) const
Expand All @@ -138,7 +153,8 @@ class HeapLocation {
&& m_heap == other.m_heap
&& m_base == other.m_base
&& m_index == other.m_index
&& m_descriptor == other.m_descriptor;
&& m_descriptor == other.m_descriptor
&& m_extraState == other.m_extraState;
}

bool isHashTableDeletedValue() const
Expand All @@ -154,6 +170,7 @@ class HeapLocation {
Node* m_base;
LazyNode m_index;
Node* m_descriptor;
void* m_extraState { nullptr };
};

struct HeapLocationHash {
Expand Down

0 comments on commit 60b4ab1

Please sign in to comment.