Changeset 274942 in webkit

Timestamp:

Mar 24, 2021, 9:56:42 AM (4 years ago)

Author:

sbarati@apple.com

Message:

r271034 added code in constant folding phase that's unreachable given current invariants of our ICs and PutByIdStatus
​https://bugs.webkit.org/show_bug.cgi?id=223625

Reviewed by Yusuke Suzuki.

The code was doing a lot of wrong things by making bad assumptions about the
invariants of PutByIdVariants. Replace PutByIdVariants never have object
property condition sets, since we always replace on the self object (and don't
look at the prototype chain). This patch clears up the code to make it
clearer what the invariants are.

With respect to the original fix about not emitting a PutByOffset for a
Replace on a Structure that has an unfired replacement watchpoint set,
that was already handled by the PutByIdStatus::computeFor variant we're
calling inside of constant folding. It will return TakesSlowPathif it
encounters a Replace where the Structure still has an unfired watchpoint.

• dfg/DFGConstantFoldingPhase.cpp:

(JSC::DFG::ConstantFoldingPhase::tryFoldAsPutByOffset):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGConstantFoldingPhase.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2021-03-24  Saam Barati  <sbarati@apple.com>
+
+        r271034 added code in constant folding phase that's unreachable given current invariants of our ICs and PutByIdStatus
+        https://bugs.webkit.org/show_bug.cgi?id=223625
+
+        Reviewed by Yusuke Suzuki.
+
+        The code was doing a lot of wrong things by making bad assumptions about the
+        invariants of PutByIdVariants. Replace PutByIdVariants never have object
+        property condition sets, since we always replace on the self object (and don't
+        look at the prototype chain). This patch clears up the code to make it
+        clearer what the invariants are.
+        
+        With respect to the original fix about not emitting a PutByOffset for a
+        Replace on a Structure that has an unfired replacement watchpoint set,
+        that was already handled by the PutByIdStatus::computeFor variant we're
+        calling inside of constant folding. It will return TakesSlowPathif it
+        encounters a Replace where the Structure still has an unfired watchpoint.
+
+        * dfg/DFGConstantFoldingPhase.cpp:
+        (JSC::DFG::ConstantFoldingPhase::tryFoldAsPutByOffset):
+
 2021-03-24  Yusuke Suzuki  <ysuzuki@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp

@@ -1420 +1420 @@
         TransitionVector transitions;
         for (const PutByIdVariant& variant : status.variants()) {
-            for (const ObjectPropertyCondition& condition : variant.conditionSet()) {
-                if (m_graph.watchCondition(condition))
-                    continue;
-
-                Structure* structure = condition.object()->structure(m_graph.m_vm);
-                if (!condition.structureEnsuresValidity(structure))
-                    return;
-
-                if (variant.kind() == PutByIdVariant::Replace) {
-                    auto* watchpoints = structure->propertyReplacementWatchpointSet(condition.offset());
-                    if (!watchpoints || watchpoints->isStillValid())
+            if (variant.kind() == PutByIdVariant::Transition) {
+                for (const ObjectPropertyCondition& condition : variant.conditionSet()) {
+                    if (m_graph.watchCondition(condition))
+                        continue;
+
+                    Structure* structure = condition.object()->structure(m_graph.m_vm);
+                    if (!condition.structureEnsuresValidity(structure))
                         return;
-                }
-
-                m_insertionSet.insertNode(
-                    indexInBlock, SpecNone, CheckStructure, node->origin,
-                    OpInfo(m_graph.addStructureSet(structure)),
-                    m_insertionSet.insertConstantForUse(
-                        indexInBlock, node->origin, condition.object(), KnownCellUse));
-            }
-
-            if (variant.kind() == PutByIdVariant::Transition) {
+
+                    m_insertionSet.insertNode(
+                        indexInBlock, SpecNone, CheckStructure, node->origin,
+                        OpInfo(m_graph.addStructureSet(structure)),
+                        m_insertionSet.insertConstantForUse(
+                            indexInBlock, node->origin, condition.object(), KnownCellUse));
+                }
+
                 ASSERT(privateFieldPutKind.isNone() || privateFieldPutKind.isDefine());
                 RegisteredStructure newStructure = m_graph.registerStructure(variant.newStructure());
@@ -1451 +1445 @@
                 ASSERT(variant.kind() == PutByIdVariant::Replace);
                 ASSERT(privateFieldPutKind.isNone() || privateFieldPutKind.isSet());
+                DFG_ASSERT(m_graph, node, variant.conditionSet().isEmpty());
                 newSet.merge(*m_graph.addStructureSet(variant.oldStructure()));
             }