Skip to content

Commit

Permalink
AirFixObviousSpills should be optimized
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=228052

Reviewed by Yusuke Suzuki.

There were two problems with AirFixObviousSpills:
- merge() had a quadratic blow-up, as for each element in a vector, it was searching it in a different vector.
- it would visit blocks even when their state at head had not changed.

I fixed the first problem by making sure that the vectors are sorted before calling merge, and making use of that invariant in the search of the vectors
(see filterVectorAgainst)
This reduced the total time spent in that phase from 390ms to 230ms, and the worst case time spent in that phase for one function from 100ms to 30ms (all of the results in this Changelog are for JetStream2 on a M1 MBP).

I fixed the second problem even more easily by adding a m_shouldVisit BitVector. I also moved the m_wasVisited boolean that was in State to a m_notBottom BitVector for simplicity and symmetry.
That change further reduced the total/max time from 230ms/30ms to 140ms/16ms.

* b3/air/AirFixObviousSpills.cpp:



Canonical link: https://commits.webkit.org/244440@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@286053 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Robin Morisset committed Nov 19, 2021
1 parent cc9bf24 commit e7fbb7a
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 43 deletions.
20 changes: 20 additions & 0 deletions Source/JavaScriptCore/ChangeLog
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,23 @@
2021-11-19 Robin Morisset <rmorisset@apple.com>

AirFixObviousSpills should be optimized
https://bugs.webkit.org/show_bug.cgi?id=228052

Reviewed by Yusuke Suzuki.

There were two problems with AirFixObviousSpills:
- merge() had a quadratic blow-up, as for each element in a vector, it was searching it in a different vector.
- it would visit blocks even when their state at head had not changed.

I fixed the first problem by making sure that the vectors are sorted before calling merge, and making use of that invariant in the search of the vectors
(see filterVectorAgainst)
This reduced the total time spent in that phase from 390ms to 230ms, and the worst case time spent in that phase for one function from 100ms to 30ms (all of the results in this Changelog are for JetStream2 on a M1 MBP).

I fixed the second problem even more easily by adding a m_shouldVisit BitVector. I also moved the m_wasVisited boolean that was in State to a m_notBottom BitVector for simplicity and symmetry.
That change further reduced the total/max time from 230ms/30ms to 140ms/16ms.

* b3/air/AirFixObviousSpills.cpp:

2021-11-18 Robin Morisset <rmorisset@apple.com>

[JSC/Air] Optimize enableMovesOnValueAndAdjacents in IRC
Expand Down
147 changes: 104 additions & 43 deletions Source/JavaScriptCore/b3/air/AirFixObviousSpills.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ class FixObviousSpills {
FixObviousSpills(Code& code)
: m_code(code)
, m_atHead(code.size())
, m_notBottom(code.size())
, m_shouldVisit(code.size())
{
}

Expand All @@ -63,31 +65,43 @@ class FixObviousSpills {
private:
void computeAliases()
{
m_atHead[m_code[0]].wasVisited = true;
m_notBottom.quickSet(0);
m_shouldVisit.quickSet(0);

bool changed = true;
while (changed) {
changed = false;

for (BasicBlock* block : m_code) {
for (unsigned blockIndex : m_shouldVisit) {
m_shouldVisit.quickClear(blockIndex);
BasicBlock* block = m_code[blockIndex];
ASSERT(m_notBottom.quickGet(blockIndex));
m_block = block;
m_state = m_atHead[block];
if (!m_state.wasVisited)
continue;

if (AirFixObviousSpillsInternal::verbose)
dataLog("Executing block ", *m_block, ": ", m_state, "\n");

for (m_instIndex = 0; m_instIndex < block->size(); ++m_instIndex)
executeInst();

// Before we call merge we must make sure that the two states are sorted.
m_state.sort();

for (BasicBlock* successor : block->successorBlocks()) {
unsigned successorIndex = successor->index();
State& toState = m_atHead[successor];
if (toState.wasVisited)
changed |= toState.merge(m_state);
else {
if (m_notBottom.quickGet(successorIndex)) {
bool changedAtSuccessorHead = toState.merge(m_state);
if (changedAtSuccessorHead) {
changed = true;
m_shouldVisit.quickSet(successorIndex);
}
} else { // The state at head of successor is bottom
toState = m_state;
changed = true;
m_notBottom.quickSet(successorIndex);
m_shouldVisit.quickSet(successorIndex);
}
}
}
Expand All @@ -99,7 +113,7 @@ class FixObviousSpills {
for (BasicBlock* block : m_code) {
m_block = block;
m_state = m_atHead[block];
RELEASE_ASSERT(m_state.wasVisited);
RELEASE_ASSERT(m_notBottom.quickGet(block->index()));

for (m_instIndex = 0; m_instIndex < block->size(); ++m_instIndex) {
fixInst();
Expand Down Expand Up @@ -341,6 +355,11 @@ class FixObviousSpills {
&& constant == other.constant;
}

bool operator<(const RegConst& other) const
{
return reg < other.reg || (reg == other.reg && constant < other.constant);
}

void dump(PrintStream& out) const
{
out.print(reg, "->", constant);
Expand Down Expand Up @@ -380,6 +399,12 @@ class FixObviousSpills {
&& mode == other.mode;
}

bool operator<(const RegSlot& other) const
{
// We ignore `mode` on purpose, see merge() for how we deal with it.
return slot < other.slot || (slot == other.slot && reg < other.reg);
}

void dump(PrintStream& out) const
{
out.print(pointerDump(slot), "->", reg);
Expand Down Expand Up @@ -423,6 +448,11 @@ class FixObviousSpills {
&& constant == other.constant;
}

bool operator<(const SlotConst& other) const
{
return slot < other.slot || (slot == other.slot && constant < other.constant);
}

void dump(PrintStream& out) const
{
out.print(pointerDump(slot), "->", constant);
Expand All @@ -435,15 +465,24 @@ class FixObviousSpills {
struct State {
void addAlias(const RegConst& newAlias)
{
return regConst.append(newAlias);
regConst.append(newAlias);
#if ASSERT_ENABLED
m_isSorted = false;
#endif
}
void addAlias(const RegSlot& newAlias)
{
return regSlot.append(newAlias);
regSlot.append(newAlias);
#if ASSERT_ENABLED
m_isSorted = false;
#endif
}
void addAlias(const SlotConst& newAlias)
{
return slotConst.append(newAlias);
slotConst.append(newAlias);
#if ASSERT_ENABLED
m_isSorted = false;
#endif
}

bool contains(const RegConst& alias)
Expand Down Expand Up @@ -544,41 +583,59 @@ class FixObviousSpills {
}
}

bool merge(const State& other)
void sort()
{
bool changed = false;

changed |= !!regConst.removeAllMatching(
[&] (RegConst& alias) -> bool {
const RegConst* otherAlias = other.getRegConst(alias.reg);
if (!otherAlias)
return true;
if (alias.constant != otherAlias->constant)
return true;
return false;
});
std::sort(regConst.begin(), regConst.end(), [] (const RegConst& a, const RegConst& b) {
return a < b;
});
std::sort(slotConst.begin(), slotConst.end(), [] (const SlotConst& a, const SlotConst& b) {
return a < b;
});
std::sort(regSlot.begin(), regSlot.end(), [] (const RegSlot& a, const RegSlot& b) {
return a < b;
});
#if ASSERT_ENABLED
m_isSorted = true;
#endif
}

changed |= !!slotConst.removeAllMatching(
[&] (SlotConst& alias) -> bool {
const SlotConst* otherAlias = other.getSlotConst(alias.slot);
if (!otherAlias)
return true;
if (alias.constant != otherAlias->constant)
// Takes two sorted vectors, for each element in the first, it looks for the first element in the second which is not smaller.
// If such an element exist, call f on both the element from the first vector and this element.
// Remove the element from the first vector unless f returned true (so f says whether to keep the element)
// Returns true if any element has been removed.
template<typename T, typename Func>
static bool filterVectorAgainst(Vector<T>& own, const Vector<T>& other, Func f)
{
const T* it = other.begin();
const T* end = other.end();
return !!own.removeAllMatching(
[&] (T& alias) {
it = std::find_if_not(it, end, [&] (const T& otherAlias) {
return otherAlias < alias;
});
if (it == end)
return true;
return false;
});
return !f(alias, *it);
});
}

changed |= !!regSlot.removeAllMatching(
[&] (RegSlot& alias) -> bool {
const RegSlot* otherAlias = other.getRegSlot(alias.reg, alias.slot);
if (!otherAlias)
return true;
if (alias.mode != RegSlot::Match32 && alias.mode != otherAlias->mode) {
alias.mode = RegSlot::Match32;
changed = true;
}
bool merge(const State& other)
{
ASSERT(m_isSorted);
ASSERT(other.m_isSorted);
bool changed = false;

changed |= filterVectorAgainst(regConst, other.regConst, [](RegConst& a, const RegConst& b) { return a == b; });
changed |= filterVectorAgainst(slotConst, other.slotConst, [](SlotConst& a, const SlotConst& b) { return a == b; });
changed |= filterVectorAgainst(regSlot, other.regSlot, [&](RegSlot& alias, const RegSlot& otherAlias) {
if (alias.reg != otherAlias.reg || alias.slot != otherAlias.slot)
return false;
});
if (alias.mode != RegSlot::Match32 && alias.mode != otherAlias.mode) {
alias.mode = RegSlot::Match32;
changed = true;
}
return true;
});

return changed;
}
Expand All @@ -587,18 +644,22 @@ class FixObviousSpills {
{
out.print(
"{regConst = [", listDump(regConst), "], slotConst = [", listDump(slotConst),
"], regSlot = [", listDump(regSlot), "], wasVisited = ", wasVisited, "}");
"], regSlot = [", listDump(regSlot), "]}");
}

Vector<RegConst> regConst;
Vector<SlotConst> slotConst;
Vector<RegSlot> regSlot;
bool wasVisited { false };
#if ASSERT_ENABLED
bool m_isSorted { true };
#endif
};

Code& m_code;
IndexMap<BasicBlock*, State> m_atHead;
State m_state;
BitVector m_notBottom;
BitVector m_shouldVisit;
BasicBlock* m_block { nullptr };
unsigned m_instIndex { 0 };
};
Expand Down

0 comments on commit e7fbb7a

Please sign in to comment.