AI rule for PutById can only observe transitions when it watches the condition
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Jan 2020 09:20:48 +0000 (09:20 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 3 Jan 2020 09:20:48 +0000 (09:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=205697
<rdar://problem/56814254>

Reviewed by Yusuke Suzuki.

JSTests:

* stress/only-transition-structures-for-put-by-id-in-AI-if-watchable.js: Added.

Source/JavaScriptCore:

There was a bug in AI where we were capturing a PutByIdStatus and
emitting a structure transition in AI state based on the variants inside this
PutByIdStatus. This, in principal, is a valid static analysis to perform.
However, we can only do this if we ensure that the snapshot we have in the
PutByIdStatus holds at runtime. We can do this by watching the property conditions
for the various variants. AI forgot to watch these conditions. This patch fixes that.
In practice, this also means we need to be slightly more strict about stating to
AI when we transition since some object property conditions aren't watchable, and need
to be verified at runtime via structure checks. This is ok in practice, since
we'll emit the code to do that inside constant folding (constant folding was
already doing this), which will continue to report the precise transition in
the abstract state.

* dfg/DFGAbstractInterpreter.h:
* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@253991 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/only-transition-structures-for-put-by-id-in-AI-if-watchable.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp

index 791da0f..1d9aa64 100644 (file)
@@ -1,3 +1,13 @@
+2020-01-03  Saam Barati  <sbarati@apple.com>
+
+        AI rule for PutById can only observe transitions when it watches the condition
+        https://bugs.webkit.org/show_bug.cgi?id=205697
+        <rdar://problem/56814254>
+
+        Reviewed by Yusuke Suzuki.
+
+        * stress/only-transition-structures-for-put-by-id-in-AI-if-watchable.js: Added.
+
 2020-01-02  Yusuke Suzuki  <ysuzuki@apple.com>
 
         REGRESSION (r253867): Six test262 tests broken
diff --git a/JSTests/stress/only-transition-structures-for-put-by-id-in-AI-if-watchable.js b/JSTests/stress/only-transition-structures-for-put-by-id-in-AI-if-watchable.js
new file mode 100644 (file)
index 0000000..2b5d93d
--- /dev/null
@@ -0,0 +1,23 @@
+//@ runDefault("--useAccessInlining=0", "--jitPolicyScale=0")
+
+let s = `
+function Ctor() {
+  this.b = 0;
+}
+
+function test2() {
+  let a = new Ctor();
+  ~a.b;
+}
+
+test2();
+test2();
+gc();
+test2();
+Object.defineProperty(Ctor.prototype, 'b', {});
+Object.defineProperty(Ctor.prototype, '0', {});
+test2();
+`
+for (let i = 0; i < 1000; i++) {
+    runString(s);
+}
index 3e5966f..d9755df 100644 (file)
@@ -1,3 +1,30 @@
+2020-01-03  Saam Barati  <sbarati@apple.com>
+
+        AI rule for PutById can only observe transitions when it watches the condition
+        https://bugs.webkit.org/show_bug.cgi?id=205697
+        <rdar://problem/56814254>
+
+        Reviewed by Yusuke Suzuki.
+
+        There was a bug in AI where we were capturing a PutByIdStatus and
+        emitting a structure transition in AI state based on the variants inside this
+        PutByIdStatus. This, in principal, is a valid static analysis to perform.
+        However, we can only do this if we ensure that the snapshot we have in the
+        PutByIdStatus holds at runtime. We can do this by watching the property conditions
+        for the various variants. AI forgot to watch these conditions. This patch fixes that.
+        In practice, this also means we need to be slightly more strict about stating to
+        AI when we transition since some object property conditions aren't watchable, and need
+        to be verified at runtime via structure checks. This is ok in practice, since
+        we'll emit the code to do that inside constant folding (constant folding was
+        already doing this), which will continue to report the precise transition in
+        the abstract state.
+
+        * dfg/DFGAbstractInterpreter.h:
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGConstantFoldingPhase.cpp:
+        (JSC::DFG::ConstantFoldingPhase::foldConstants):
+
 2020-01-02  Yusuke Suzuki  <ysuzuki@apple.com> and Simon Fraser  <simon.fraser@apple.com>
 
         Experiment: create lots of different malloc zones for easier accounting of memory use
index d7ffb6c..e296102 100644 (file)
@@ -215,9 +215,9 @@ public:
     
     void filterICStatus(Node*);
     
-private:
     void clobberWorld();
     void didFoldClobberWorld();
+private:
     
     bool handleConstantBinaryBitwiseOp(Node*);
 
@@ -228,7 +228,9 @@ private:
     void didFoldClobberStructures();
     
     void observeTransition(unsigned indexInBlock, RegisteredStructure from, RegisteredStructure to);
+public:
     void observeTransitions(unsigned indexInBlock, const TransitionVector&);
+private:
     
     enum BooleanResult {
         UnknownBooleanResult,
index ed2507c..71c4d06 100644 (file)
@@ -3768,12 +3768,22 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                 m_graph.identifiers()[node->identifierNumber()],
                 node->op() == PutByIdDirect);
 
+            bool allGood = true;
             if (status.isSimple()) {
                 RegisteredStructureSet newSet;
                 TransitionVector transitions;
                 
-                for (unsigned i = status.numVariants(); i--;) {
-                    const PutByIdVariant& variant = status[i];
+                for (const PutByIdVariant& variant : status.variants()) {
+                    for (const ObjectPropertyCondition& condition : variant.conditionSet()) {
+                        if (!m_graph.watchCondition(condition)) {
+                            allGood = false;
+                            break;
+                        }
+                    }
+
+                    if (!allGood)
+                        break;
+
                     if (variant.kind() == PutByIdVariant::Transition) {
                         RegisteredStructure newStructure = m_graph.registerStructure(variant.newStructure());
                         transitions.append(
@@ -3789,11 +3799,13 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
                 if (status.numVariants() == 1 || m_graph.m_plan.isFTL())
                     m_state.setShouldTryConstantFolding(true);
                 
-                didFoldClobberWorld();
-                observeTransitions(clobberLimit, transitions);
-                if (forNode(node->child1()).changeStructure(m_graph, newSet) == Contradiction)
-                    m_state.setIsValid(false);
-                break;
+                if (allGood) {
+                    didFoldClobberWorld();
+                    observeTransitions(clobberLimit, transitions);
+                    if (forNode(node->child1()).changeStructure(m_graph, newSet) == Contradiction)
+                        m_state.setIsValid(false);
+                    break;
+                }
             }
         }
         
index 185366d..b8af520 100644 (file)
@@ -606,18 +606,15 @@ private:
                 AbstractValue baseValue = m_state.forNode(child);
                 AbstractValue valueValue = m_state.forNode(node->child2());
 
-                m_interpreter.execute(indexInBlock); // Push CFA over this node after we get the state before.
-                alreadyHandled = true; // Don't allow the default constant folder to do things to this.
-
                 if (!baseValue.m_structure.isFinite())
                     break;
-                
+
                 PutByIdStatus status = PutByIdStatus::computeFor(
                     m_graph.globalObjectFor(origin.semantic),
                     baseValue.m_structure.toStructureSet(),
                     m_graph.identifiers()[identifierNumber],
                     node->op() == PutByIdDirect);
-                
+
                 if (!status.isSimple())
                     break;
 
@@ -629,9 +626,9 @@ private:
                 changed = true;
 
                 bool allGood = true;
+                RegisteredStructureSet newSet;
+                TransitionVector transitions;
                 for (const PutByIdVariant& variant : status.variants()) {
-                    if (!allGood)
-                        break;
                     for (const ObjectPropertyCondition& condition : variant.conditionSet()) {
                         if (m_graph.watchCondition(condition))
                             continue;
@@ -648,10 +645,32 @@ private:
                             m_insertionSet.insertConstantForUse(
                                 indexInBlock, node->origin, condition.object(), KnownCellUse));
                     }
+
+                    if (!allGood)
+                        break;
+
+                    if (variant.kind() == PutByIdVariant::Transition) {
+                        RegisteredStructure newStructure = m_graph.registerStructure(variant.newStructure());
+                        transitions.append(
+                            Transition(
+                                m_graph.registerStructure(variant.oldStructureForTransition()), newStructure));
+                        newSet.add(newStructure);
+                    } else {
+                        ASSERT(variant.kind() == PutByIdVariant::Replace);
+                        newSet.merge(*m_graph.addStructureSet(variant.oldStructure()));
+                    }
                 }
 
                 if (!allGood)
                     break;
+
+                // Push CFA over this node after we get the state before.
+                m_interpreter.didFoldClobberWorld();
+                m_interpreter.observeTransitions(indexInBlock, transitions);
+                if (m_state.forNode(node->child1()).changeStructure(m_graph, newSet) == Contradiction)
+                    m_state.setIsValid(false);
+
+                alreadyHandled = true; // Don't allow the default constant folder to do things to this.
                 
                 m_insertionSet.insertNode(
                     indexInBlock, SpecNone, FilterPutByIdStatus, node->origin,