Skip to content

Commit bea1824

Browse files
ConstellationConstellation
committed
[JSC] MovHintRemoval should track exit liveness
https://bugs.webkit.org/show_bug.cgi?id=293337 rdar://151740794 Reviewed by Yijia Huang and Justin Michaud. The current MovHintRemoval's analysis looks weird. We should just do liveness analysis globally and use this information for MovHint removal. 1. "Use" is a node which may exit. When exit happens, we should keep all use of live locals at this bytecode exit location alive. 2. "Def" is MovHint. We kill the locals here. And doing fixpoint analysis and using this information to remove MovHint. Also, pruning Availability in OSRAvailabilityAnalysisPhase via bytecode liveness is wrong: they need to keep live nodes from DFG for example. 0: PutHint @x, PROP(@y) 1: OSR exit point #1 (here, loc0 is not alive) 2: -- Pruning happens -- 3: MovHint @x, loc0 4: OSR exit point #2 (here, loc0 is alive) In this case pruning point will remove (0)'s heap availability since @x is not alive from bytecode at (1), but this is wrong as we need this in (4). In this patch, we remove pruneByLiveness in DFGOSRAvailabilityAnalysisPhase. This pruning should happen by the user of DFGOSRAvailabilityAnalysisPhase instead, and it is already happening (see FTLLowerToB3's pruneByLiveness in exit site, which is right. And ObjectAllocationSinking is pruning with CombinedLiveness, this is right since it also accounts Node's liveness in addition to bytecode's liveness.). Let's just make availability just compute the availability for all things, and then we prune some of unnecessary ones at each use of this information. * Source/JavaScriptCore/dfg/DFGMovHintRemovalPhase.cpp: * Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp: (JSC::DFG::OSRAvailabilityAnalysisPhase::run): Canonical link: https://commits.webkit.org/295369@main
1 parent fa536ea commit bea1824

File tree

2 files changed

+62
-99
lines changed

2 files changed

+62
-99
lines changed

‎Source/JavaScriptCore/dfg/DFGMovHintRemovalPhase.cpp

Lines changed: 61 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,6 @@ class MovHintRemovalPhase : public Phase {
5050
MovHintRemovalPhase(Graph& graph)
5151
: Phase(graph, "MovHint removal"_s)
5252
, m_insertionSet(graph)
53-
, m_state(OperandsLike, graph.block(0)->variablesAtHead)
5453
, m_changed(false)
5554
{
5655
}
@@ -59,16 +58,64 @@ class MovHintRemovalPhase : public Phase {
5958
{
6059
dataLogIf(DFGMovHintRemovalPhaseInternal::verbose, "Graph before MovHint removal:\n", m_graph);
6160

61+
// First figure out where various locals are live. We only need to care when we exit.
62+
// So,
63+
// 1. When exiting, mark all live variables alive. And we completely clear dead variables at that time.
64+
// 2. When performing MovHint, it is def and it kills the previous live variable.
65+
BlockMap<Operands<bool>> liveAtHead(m_graph);
66+
BlockMap<Operands<bool>> liveAtTail(m_graph);
67+
68+
for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
69+
liveAtHead[block] = Operands<bool>(OperandsLike, block->variablesAtHead, false);
70+
liveAtTail[block] = Operands<bool>(OperandsLike, block->variablesAtHead, false);
71+
}
72+
73+
bool changed;
74+
do {
75+
changed = false;
76+
for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
77+
BasicBlock* block = m_graph.block(blockIndex);
78+
if (!block)
79+
continue;
80+
81+
Operands<bool> live = liveAtTail[block];
82+
for (unsigned nodeIndex = block->size(); nodeIndex--;) {
83+
Node* node = block->at(nodeIndex);
84+
if (node->op() == MovHint)
85+
live.operand(node->unlinkedOperand()) = false;
86+
87+
if (mayExit(m_graph, node) != DoesNotExit) {
88+
m_graph.forAllLiveInBytecode(
89+
node->origin.forExit,
90+
[&](Operand reg) {
91+
live.operand(reg) = true;
92+
});
93+
}
94+
}
95+
96+
if (live == liveAtHead[block])
97+
continue;
98+
99+
liveAtHead[block] = live;
100+
changed = true;
101+
102+
for (BasicBlock* predecessor : block->predecessors) {
103+
for (size_t i = live.size(); i--;)
104+
liveAtTail[predecessor][i] |= live[i];
105+
}
106+
}
107+
} while (changed);
108+
62109
for (BasicBlock* block : m_graph.blocksInNaturalOrder())
63-
handleBlock(block);
110+
handleBlock(block, liveAtTail[block]);
64111

65112
m_insertionSet.execute(m_graph.block(0));
66113

67114
return m_changed;
68115
}
69116

70117
private:
71-
void handleBlock(BasicBlock* block)
118+
void handleBlock(BasicBlock* block, const Operands<bool>& liveAtTail)
72119
{
73120
dataLogLnIf(DFGMovHintRemovalPhaseInternal::verbose, "Handing block ", pointerDump(block));
74121

@@ -78,70 +125,17 @@ class MovHintRemovalPhase : public Phase {
78125
// local, then it means there was no exit between the local's death and the MovHint - i.e.
79126
// the MovHint is unnecessary.
80127

81-
Epoch currentEpoch = Epoch::first();
82-
83-
m_state.fill(Epoch());
84-
85-
// This is a heuristic. We found cases that BB ends with Jump, and successor no longer have liveness for locals.
86-
// However, at the end of the current BB, it is alive and we failed to kill that MovHint while we do not have exits.
87-
// This is a work-around for that case, we chase only one successor and collect a bit more precise liveness information.
88-
if (block->terminal()->isJump()) {
89-
auto* successor = block->successor(0);
90-
91-
m_graph.forAllLiveInBytecode(
92-
successor->terminal()->origin.forExit,
93-
[&] (Operand reg) {
94-
m_state.operand(reg) = currentEpoch;
95-
});
128+
Operands<bool> live = liveAtTail;
96129

97-
// Assume that blocks after we exit.
98-
currentEpoch.bump();
99-
100-
for (unsigned nodeIndex = successor->size(); nodeIndex--;) {
101-
Node* node = successor->at(nodeIndex);
102-
103-
if (node->op() == MovHint)
104-
m_state.operand(node->unlinkedOperand()) = Epoch();
105-
106-
if (mayExit(m_graph, node) != DoesNotExit)
107-
currentEpoch.bump();
108-
109-
Node* before = block->terminal();
110-
if (nodeIndex)
111-
before = successor->at(nodeIndex - 1);
112-
113-
forAllKilledOperands(
114-
m_graph, before, node,
115-
[&] (Operand operand) {
116-
// This function is a bit sloppy - it might claim to kill a local even if
117-
// it's still live after. We need to protect against that.
118-
if (!!m_state.operand(operand))
119-
return;
120-
121-
dataLogLnIf(DFGMovHintRemovalPhaseInternal::verbose, " Killed operand at ", node, ": ", operand);
122-
m_state.operand(operand) = currentEpoch;
123-
});
124-
}
125-
} else {
126-
m_graph.forAllLiveInBytecode(
127-
block->terminal()->origin.forExit,
128-
[&] (Operand reg) {
129-
m_state.operand(reg) = currentEpoch;
130-
});
131-
132-
// Assume that blocks after we exit.
133-
currentEpoch.bump();
134-
}
135-
136-
dataLogLnIf(DFGMovHintRemovalPhaseInternal::verbose, " Locals at ", block->terminal()->origin.forExit, ": ", m_state);
130+
dataLogLnIf(DFGMovHintRemovalPhaseInternal::verbose, " Locals at ", block->terminal()->origin.forExit, ": ", live);
137131

138132
for (unsigned nodeIndex = block->size(); nodeIndex--;) {
139133
Node* node = block->at(nodeIndex);
140134

141135
if (node->op() == MovHint) {
142-
Epoch localEpoch = m_state.operand(node->unlinkedOperand());
143-
dataLogLnIf(DFGMovHintRemovalPhaseInternal::verbose, " At ", node, " (", node->unlinkedOperand(), "): current = ", currentEpoch, ", local = ", localEpoch);
144-
if (!localEpoch || localEpoch == currentEpoch) {
136+
bool isAlive = live.operand(node->unlinkedOperand());
137+
dataLogLnIf(DFGMovHintRemovalPhaseInternal::verbose, " At ", node, " (", node->unlinkedOperand(), "): live: ", isAlive);
138+
if (!isAlive) {
145139
// Now, MovHint will put bottom value to dead locals. This means that if you insert a new DFG node which introduce
146140
// a new OSR exit, then it gets confused with the already-determined-dead locals. So this phase runs at very end of
147141
// DFG pipeline, and we do not insert a node having a new OSR exit (if it is existing OSR exit, or if it does not exit,
@@ -154,31 +148,21 @@ class MovHintRemovalPhase : public Phase {
154148
node->child1() = Edge(constant, useKind);
155149
m_changed = true;
156150
}
157-
m_state.operand(node->unlinkedOperand()) = Epoch();
151+
live.operand(node->unlinkedOperand()) = false;
158152
}
159153

160-
if (mayExit(m_graph, node) != DoesNotExit)
161-
currentEpoch.bump();
162-
163-
if (nodeIndex) {
164-
forAllKilledOperands(
165-
m_graph, block->at(nodeIndex - 1), node,
166-
[&] (Operand operand) {
167-
// This function is a bit sloppy - it might claim to kill a local even if
168-
// it's still live after. We need to protect against that.
169-
if (!!m_state.operand(operand))
170-
return;
171-
172-
dataLogLnIf(DFGMovHintRemovalPhaseInternal::verbose, " Killed operand at ", node, ": ", operand);
173-
m_state.operand(operand) = currentEpoch;
154+
if (mayExit(m_graph, node) != DoesNotExit) {
155+
m_graph.forAllLiveInBytecode(
156+
node->origin.forExit,
157+
[&](Operand reg) {
158+
live.operand(reg) = true;
174159
});
175160
}
176161
}
177162
}
178163

179164
InsertionSet m_insertionSet;
180165
UncheckedKeyHashMap<std::underlying_type_t<UseKind>, Node*, WTF::IntHash<std::underlying_type_t<UseKind>>, WTF::UnsignedWithZeroKeyHashTraits<std::underlying_type_t<UseKind>>> m_constants;
181-
Operands<Epoch> m_state;
182166
bool m_changed;
183167
};
184168

‎Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.cpp

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -72,16 +72,6 @@ class OSRAvailabilityAnalysisPhase : public Phase {
7272
dataLogLn("Tail: ", availabilityAtTail(block));
7373
};
7474

75-
auto dumpBytecodeLivenessAtHead = [&] (BasicBlock* block) {
76-
dataLog("Live: ");
77-
m_graph.forAllLiveInBytecode(
78-
block->at(0)->origin.forExit,
79-
[&] (Operand operand) {
80-
dataLog(operand, " ");
81-
});
82-
dataLogLn("");
83-
};
84-
8575
LocalOSRAvailabilityCalculator calculator(m_graph);
8676
bool changed;
8777
do {
@@ -113,17 +103,6 @@ class OSRAvailabilityAnalysisPhase : public Phase {
113103
BasicBlock* successor = block->successor(successorIndex);
114104
availabilityAtHead(successor).merge(calculator.m_availability);
115105
}
116-
117-
for (unsigned successorIndex = block->numSuccessors(); successorIndex--;) {
118-
BasicBlock* successor = block->successor(successorIndex);
119-
availabilityAtHead(successor).pruneByLiveness(
120-
m_graph, successor->at(0)->origin.forExit);
121-
if (verbose) {
122-
dataLogLn("After pruning Block #", successor->index);
123-
dumpAvailability(successor);
124-
dumpBytecodeLivenessAtHead(successor);
125-
}
126-
}
127106
}
128107
} while (changed);
129108

@@ -134,7 +113,7 @@ class OSRAvailabilityAnalysisPhase : public Phase {
134113

135114
for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
136115
Node* node = block->at(nodeIndex);
137-
if (node->origin.exitOK) {
116+
if (mayExit(m_graph, node) != DoesNotExit) {
138117
// If we're allowed to exit here, the heap must be in a state
139118
// where exiting wouldn't crash. These particular fields are
140119
// required for correctness because we use them during OSR exit

0 commit comments

Comments
 (0)