Modified: trunk/Source/_javascript_Core/ChangeLog (230974 => 230975)
--- trunk/Source/_javascript_Core/ChangeLog 2018-04-24 22:11:53 UTC (rev 230974)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-04-24 22:29:39 UTC (rev 230975)
@@ -1,5 +1,36 @@
2018-04-24 Filip Pizlo <[email protected]>
+ MultiByOffset should emit one fewer branches in the case that the set of structures is proved already
+ https://bugs.webkit.org/show_bug.cgi?id=184923
+
+ Reviewed by Saam Barati.
+
+ If we have a MultiGetByOffset or MultiPutByOffset over a structure set that we've already proved
+ (i.e. we know that the object has one of those structures), then previously we would still emit a
+ switch with a case per structure along with a default case. That would mean one extra redundant
+ branch to check that whatever structure we wound up with belongs to the set. In that case, we
+ were already making the default case be an Oops.
+
+ One possible solution would be to say that the default case being Oops means that B3 doesn't need
+ to emit the extra branch. But that would require having B3 exploit the fact that Oops is known to
+ be unreachable. Although B3 IR semantics (webkit.org/docs/b3/intermediate-representation.html)
+ seem to allow this, I don't particularly like that style of optimization. I like Oops to mean
+ trap.
+
+ So, this patch makes FTL lowering turn one of the cases into the default, explicitly removing the
+ extra branch.
+
+ This is not a speed-up. But it makes the B3 IR for MultiByOffset a lot simpler, which should make
+ it easier to implement B3-level optimizations for MultiByOffset. It also makes the IR easier to
+ read.
+
+ * ftl/FTLLowerDFGToB3.cpp:
+ (JSC::FTL::DFG::LowerDFGToB3::compileMultiGetByOffset):
+ (JSC::FTL::DFG::LowerDFGToB3::compileMultiPutByOffset):
+ (JSC::FTL::DFG::LowerDFGToB3::emitSwitchForMultiByOffset):
+
+2018-04-24 Filip Pizlo <[email protected]>
+
DFG CSE should know how to decay a MultiGetByOffset
https://bugs.webkit.org/show_bug.cgi?id=159859
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (230974 => 230975)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-04-24 22:11:53 UTC (rev 230974)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2018-04-24 22:29:39 UTC (rev 230975)
@@ -6438,14 +6438,6 @@
MultiGetByOffsetData& data = ""
- if (data.cases.isEmpty()) {
- // Protect against creating a Phi function with zero inputs. LLVM didn't like that.
- // It's not clear if this is needed anymore.
- // FIXME: https://bugs.webkit.org/show_bug.cgi?id=154382
- terminate(BadCache);
- return;
- }
-
Vector<LBasicBlock, 2> blocks(data.cases.size());
for (unsigned i = data.cases.size(); i--;)
blocks[i] = m_out.newBlock();
@@ -6462,8 +6454,8 @@
cases.append(SwitchCase(weakStructureID(structure), blocks[i], Weight(1)));
}
}
- m_out.switchInstruction(
- m_out.load32(base, m_heaps.JSCell_structureID), cases, exit, Weight(0));
+ bool structuresChecked = m_interpreter.forNode(m_node->child1()).m_structure.isSubsetOf(baseSet);
+ emitSwitchForMultiByOffset(base, structuresChecked, cases, exit);
LBasicBlock lastNext = m_out.m_nextBlock;
@@ -6504,7 +6496,7 @@
}
m_out.appendTo(exit, continuation);
- if (!m_interpreter.forNode(m_node->child1()).m_structure.isSubsetOf(baseSet))
+ if (!structuresChecked)
speculate(BadCache, noValue(), nullptr, m_out.booleanTrue);
m_out.unreachable();
@@ -6544,8 +6536,8 @@
cases.append(SwitchCase(weakStructureID(structure), blocks[i], Weight(1)));
}
}
- m_out.switchInstruction(
- m_out.load32(base, m_heaps.JSCell_structureID), cases, exit, Weight(0));
+ bool structuresChecked = m_interpreter.forNode(m_node->child1()).m_structure.isSubsetOf(baseSet);
+ emitSwitchForMultiByOffset(base, structuresChecked, cases, exit);
LBasicBlock lastNext = m_out.m_nextBlock;
@@ -6587,7 +6579,7 @@
}
m_out.appendTo(exit, continuation);
- if (!m_interpreter.forNode(m_node->child1()).m_structure.isSubsetOf(baseSet))
+ if (!structuresChecked)
speculate(BadCache, noValue(), nullptr, m_out.booleanTrue);
m_out.unreachable();
@@ -12030,6 +12022,29 @@
setJSValue(patchpoint);
}
+ void emitSwitchForMultiByOffset(LValue base, bool structuresChecked, Vector<SwitchCase, 2>& cases, LBasicBlock exit)
+ {
+ if (cases.isEmpty()) {
+ m_out.jump(exit);
+ return;
+ }
+
+ if (structuresChecked) {
+ std::sort(
+ cases.begin(), cases.end(),
+ [&] (const SwitchCase& a, const SwitchCase& b) -> bool {
+ return a.value()->asInt() < b.value()->asInt();
+ });
+ SwitchCase last = cases.takeLast();
+ m_out.switchInstruction(
+ m_out.load32(base, m_heaps.JSCell_structureID), cases, last.target(), Weight(0));
+ return;
+ }
+
+ m_out.switchInstruction(
+ m_out.load32(base, m_heaps.JSCell_structureID), cases, exit, Weight(0));
+ }
+
void compareEqObjectOrOtherToObject(Edge leftChild, Edge rightChild)
{
LValue rightCell = lowCell(rightChild);
