Modified: trunk/Source/_javascript_Core/ChangeLog (131269 => 131270)
--- trunk/Source/_javascript_Core/ChangeLog 2012-10-14 19:58:26 UTC (rev 131269)
+++ trunk/Source/_javascript_Core/ChangeLog 2012-10-14 20:22:17 UTC (rev 131270)
@@ -1,5 +1,29 @@
2012-10-14 Filip Pizlo <[email protected]>
+ DFG structure check hoisting should attempt to ignore side effects and make transformations that are sound even in their presence
+ https://bugs.webkit.org/show_bug.cgi?id=99262
+
+ Reviewed by Oliver Hunt.
+
+ This hugely simplifies the structure check hoisting phase. It will no longer be necessary
+ to modify it when the effectfulness of operations changes. This also enables the hoister
+ to hoist effectful things in the future.
+
+ The downside is that the hoister may end up adding strictly more checks than were present
+ in the original code, if the code truly has a lot of side-effects. I don't see evidence
+ of this happening. This patch does have some speed-ups and some slow-downs, but is
+ neutral in the average, and the slow-downs do not appear to have more structure checks
+ than ToT.
+
+ * dfg/DFGStructureCheckHoistingPhase.cpp:
+ (JSC::DFG::StructureCheckHoistingPhase::run):
+ (JSC::DFG::StructureCheckHoistingPhase::noticeStructureCheck):
+ (StructureCheckHoistingPhase):
+ (CheckData):
+ (JSC::DFG::StructureCheckHoistingPhase::CheckData::CheckData):
+
+2012-10-14 Filip Pizlo <[email protected]>
+
Fix the build of universal binary with ARMv7s of _javascript_Core
* llint/LLIntOfflineAsmConfig.h:
Modified: trunk/Source/_javascript_Core/dfg/DFGStructureCheckHoistingPhase.cpp (131269 => 131270)
--- trunk/Source/_javascript_Core/dfg/DFGStructureCheckHoistingPhase.cpp 2012-10-14 19:58:26 UTC (rev 131269)
+++ trunk/Source/_javascript_Core/dfg/DFGStructureCheckHoistingPhase.cpp 2012-10-14 20:22:17 UTC (rev 131270)
@@ -215,125 +215,6 @@
}
}
- // Identify the set of variables that are live across a structure clobber.
-
- Operands<VariableAccessData*> live(
- m_graph.m_blocks[0]->variablesAtTail.numberOfArguments(),
- m_graph.m_blocks[0]->variablesAtTail.numberOfLocals());
- for (BlockIndex blockIndex = 0; blockIndex < m_graph.m_blocks.size(); ++blockIndex) {
- BasicBlock* block = m_graph.m_blocks[blockIndex].get();
- if (!block)
- continue;
- ASSERT(live.numberOfArguments() == block->variablesAtTail.numberOfArguments());
- ASSERT(live.numberOfLocals() == block->variablesAtTail.numberOfLocals());
- for (unsigned i = live.size(); i--;) {
- NodeIndex indexAtTail = block->variablesAtTail[i];
- VariableAccessData* variable;
- if (indexAtTail == NoNode)
- variable = 0;
- else
- variable = m_graph[indexAtTail].variableAccessData();
- live[i] = variable;
- }
- for (unsigned indexInBlock = block->size(); indexInBlock--;) {
- NodeIndex nodeIndex = block->at(indexInBlock);
- Node& node = m_graph[nodeIndex];
- if (!node.shouldGenerate())
- continue;
- switch (node.op()) {
- case GetLocal:
- case Flush:
- // This is a birth.
- live.operand(node.local()) = node.variableAccessData();
- break;
-
- case SetLocal:
- case SetArgument:
- ASSERT(live.operand(node.local())); // Must be live.
- ASSERT(live.operand(node.local()) == node.variableAccessData()); // Must have the variable we expected.
- // This is a death.
- live.operand(node.local()) = 0;
- break;
-
- // Use the CFA's notion of what clobbers the world.
- case ValueAdd:
- if (m_graph.addShouldSpeculateInteger(node))
- break;
- if (Node::shouldSpeculateNumber(m_graph[node.child1()], m_graph[node.child2()]))
- break;
- clobber(live);
- break;
-
- case CompareLess:
- case CompareLessEq:
- case CompareGreater:
- case CompareGreaterEq:
- case CompareEq: {
- Node& left = m_graph[node.child1()];
- Node& right = m_graph[node.child2()];
- if (Node::shouldSpeculateInteger(left, right))
- break;
- if (Node::shouldSpeculateNumber(left, right))
- break;
- if (node.op() == CompareEq) {
- if ((m_graph.isConstant(node.child1().index())
- && m_graph.valueOfJSConstant(node.child1().index()).isNull())
- || (m_graph.isConstant(node.child2().index())
- && m_graph.valueOfJSConstant(node.child2().index()).isNull()))
- break;
-
- if (Node::shouldSpeculateFinalObject(left, right))
- break;
- if (Node::shouldSpeculateArray(left, right))
- break;
- if (left.shouldSpeculateFinalObject() && right.shouldSpeculateFinalObjectOrOther())
- break;
- if (right.shouldSpeculateFinalObject() && left.shouldSpeculateFinalObjectOrOther())
- break;
- if (left.shouldSpeculateArray() && right.shouldSpeculateArrayOrOther())
- break;
- if (right.shouldSpeculateArray() && left.shouldSpeculateArrayOrOther())
- break;
- }
- clobber(live);
- break;
- }
-
- case GetByVal:
- case PutByVal:
- case PutByValAlias:
- if (m_graph.byValIsPure(node))
- break;
- clobber(live);
- break;
-
- case GetMyArgumentsLengthSafe:
- case GetMyArgumentByValSafe:
- case GetById:
- case GetByIdFlush:
- case PutStructure:
- case PhantomPutStructure:
- case PutById:
- case PutByIdDirect:
- case Call:
- case Construct:
- case Resolve:
- case ResolveBase:
- case ResolveBaseStrictPut:
- case ResolveGlobal:
- case ArrayPush:
- case ArrayPop:
- case Arrayify:
- clobber(live);
- break;
-
- default:
- ASSERT(node.op() != Phi);
- break;
- }
- }
- }
-
bool changed = false;
#if DFG_ENABLE(DEBUG_PROPAGATION_VERBOSE)
@@ -343,20 +224,11 @@
dataLog("Not hoisting checks for %s because of heuristics.\n", m_graph.nameOfVariableAccessData(it->key));
continue;
}
- if (it->value.m_isClobbered && !it->value.m_structure->transitionWatchpointSetIsStillValid()) {
- dataLog("Not hoisting checks for %s because the structure is clobbered and has an invalid watchpoint set.\n", m_graph.nameOfVariableAccessData(it->key));
- continue;
- }
dataLog("Hoisting checks for %s\n", m_graph.nameOfVariableAccessData(it->key));
}
#endif // DFG_ENABLE(DEBUG_PROPAGATION_VERBOSE)
- // Make changes:
- // 1) If a variable's live range does not span a clobber, then inject structure
- // checks before the SetLocal.
- // 2) If a variable's live range spans a clobber but is watchpointable, then
- // inject structure checks before the SetLocal and replace all other structure
- // checks on that variable with structure transition watchpoints.
+ // Place CheckStructure's at SetLocal sites.
InsertionSet<NodeIndex> insertionSet;
for (BlockIndex blockIndex = 0; blockIndex < m_graph.m_blocks.size(); ++blockIndex) {
@@ -384,8 +256,6 @@
break;
if (!iter->value.m_structure)
break;
- if (iter->value.m_isClobbered && !iter->value.m_structure->transitionWatchpointSetIsStillValid())
- break;
node.ref();
@@ -420,8 +290,6 @@
break;
if (!iter->value.m_structure)
break;
- if (iter->value.m_isClobbered && !iter->value.m_structure->transitionWatchpointSetIsStillValid())
- break;
// First insert a dead SetLocal to tell OSR that the child's value should
// be dropped into this bytecode variable if the CheckStructure decides
@@ -446,28 +314,6 @@
break;
}
- case CheckStructure: {
- Node& child = m_graph[node.child1()];
- if (child.op() != GetLocal)
- break;
- HashMap<VariableAccessData*, CheckData>::iterator iter = m_map.find(child.variableAccessData());
- if (iter == m_map.end())
- break;
- if (!iter->value.m_structure)
- break;
- if (!iter->value.m_isClobbered) {
- node.setOpAndDefaultFlags(Phantom);
- ASSERT(node.refCount() == 1);
- break;
- }
- if (!iter->value.m_structure->transitionWatchpointSetIsStillValid())
- break;
- ASSERT(iter->value.m_structure == node.structureSet().singletonStructure());
- node.convertToStructureTransitionWatchpoint();
- changed = true;
- break;
- }
-
default:
break;
}
@@ -482,7 +328,7 @@
void noticeStructureCheck(VariableAccessData* variable, Structure* structure)
{
HashMap<VariableAccessData*, CheckData>::AddResult result =
- m_map.add(variable, CheckData(structure, false));
+ m_map.add(variable, CheckData(structure));
if (result.isNewEntry)
return;
if (result.iterator->value.m_structure == structure)
@@ -499,38 +345,16 @@
noticeStructureCheck(variable, set.singletonStructure());
}
- void noticeClobber(VariableAccessData* variable)
- {
- HashMap<VariableAccessData*, CheckData>::iterator iter =
- m_map.find(variable);
- if (iter == m_map.end())
- return;
- iter->value.m_isClobbered = true;
- }
-
- void clobber(const Operands<VariableAccessData*>& live)
- {
- for (size_t i = live.size(); i--;) {
- VariableAccessData* variable = live[i];
- if (!variable)
- continue;
- noticeClobber(variable);
- }
- }
-
struct CheckData {
Structure* m_structure;
- bool m_isClobbered;
CheckData()
: m_structure(0)
- , m_isClobbered(false)
{
}
- CheckData(Structure* structure, bool isClobbered)
+ CheckData(Structure* structure)
: m_structure(structure)
- , m_isClobbered(isClobbered)
{
}
};
