Attachment 359098 Details for Bug 193413 – Patch

[patch] Patch

bug-193413-20190114164535.patch (text/plain), 7.98 KB, created by Yusuke Suzuki on 2019-01-14 16:45:35 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-14 16:45:35 PST

Size: 7.98 KB

patch

obsolete

>Subversion Revision: 239955
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index cc5b39291b6d97b69d9188a35eb12dcc3fe635e6..f941ee42d83d8d1f122509b16bed41c3a7de01ca 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,42 @@
>+2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [JSC] AI should check the given constant's array type when folding GetByVal into constant
>+        https://bugs.webkit.org/show_bug.cgi?id=193413
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        If GetByVal's DFG::ArrayMode's type is Array::Double, we expect that the result of GetByVal is Double, since we already performed CheckStructure or CheckArray
>+        to ensure this array type. But this assumption on the given value becomes wrong in AI, since CheckStructure may not perform filtering. And the proven AbstractValue
>+        in GetByVal would not be expected one.
>+
>+        We have the graph before performing constant folding.
>+
>+        53:<!0:->     GetLocal(Check:Untyped:@77, JS|MustGen|UseAsOther, Array, arg2(C<Array>/FlushedCell), R:Stack(7), bc#37, ExitValid)  predicting Array
>+        54:< 1:->     JSConstant(JS|PureNum|UseAsOther|UseAsInt|ReallyWantsInt, BoolInt32, Int32: 0, bc#37, ExitValid)
>+        93:<!0:->     CheckStructure(Cell:@53, MustGen, [%C7:Array], R:JSCell_structureID, Exits, bc#37, ExitValid)
>+        94:< 1:->     GetButterfly(Check:Cell:@53, Storage|PureInt, R:JSObject_butterfly, Exits, bc#37, ExitValid)
>+        55:<!0:->     GetByVal(Check:KnownCell:@53, Check:Int32:@54, Check:Untyped:@94, Double|MustGen|VarArgs|PureInt, AnyIntAsDouble|NonIntAsdouble, Double+OriginalCopyOnWriteArray+SaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedDoubleProperties, Exits, bc#37, ExitValid)  predicting StringIdent|NonIntAsdouble
>+
>+        And 53 is converted to JSConstant in the constant folding. It leads to constant folding attempt in GetByVal.
>+
>+        53:< 1:->     JSConstant(JS|UseAsOther, Array, Weak:Object: 0x117fb4370 with butterfly 0x8000e4050 (Structure %BV:Array), StructureID: 104, bc#37, ExitValid)
>+        54:< 1:->     JSConstant(JS|PureNum|UseAsOther|UseAsInt|ReallyWantsInt, BoolInt32, Int32: 0, bc#37, ExitValid)
>+        93:<!0:->     CheckStructure(Cell:@53, MustGen, [%C7:Array], R:JSCell_structureID, Exits, bc#37, ExitValid)
>+        94:< 1:->     GetButterfly(Check:Cell:@53, Storage|PureInt, R:JSObject_butterfly, Exits, bc#37, ExitValid)
>+        55:<!0:->     GetByVal(Check:KnownCell:@53, Check:Int32:@54, Check:Untyped:@94, Double|MustGen|VarArgs|PureInt, AnyIntAsDouble|NonIntAsdouble, Double+OriginalCopyOnWriteArray+SaneChain+AsIs+Read, R:Butterfly_publicLength,IndexedDoubleProperties, Exits, bc#37, ExitValid)  predicting StringIdent|NonIntAsdouble
>+
>+        GetByVal gets constant Array from @53, and attempt to perform constant folding by leverating CoW state: if the given array's butterfly is CoW and we performed CoW array check for this GetByVal, the array would not be changed as long as the check works.
>+        However, CheckStructure for @53 does not filter anything at AI. So, if @53 is CopyOnWrite | Contiguous array (not CopyOnWrite | Double array!), GetByVal will get a JSValue. But it does not meet the requirement of GetByVal since it has Double Array mode, and says it returns Double.
>+        Here, CheckStructure is valid because structure of the constant object would be changed. What we should do is additional CoW & ArrayShape check in GetByVal when folding since this node leverages CoW's interesting feature,
>+        "If CoW array check (CheckStructure etc.) is emitted by GetByVal's DFG::ArrayMode, the content is not changed from the creation!".
>+
>+        This patch adds ArrayShape check in addition to CoW status check in GetByVal.
>+
>+        * dfg/DFGAbstractInterpreterInlines.h:
>+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
>+        * dfg/DFGAbstractValue.cpp:
>+        (JSC::DFG::AbstractValue::fixTypeForRepresentation):
>+
> 2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [JSC] Do not use asArrayModes() with Structures because it discards TypedArray information
>diff --git a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>index 2c55ac3e330630b516b3f5dc26f172436ac3d9b5..f87fcd532c12f72bedc816b9f560a1eb58bec053 100644
>--- a/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>+++ b/Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>@@ -1934,9 +1934,17 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
>                         return false;
> 
>                     Structure* structure = m_vm.getStructure(structureIDLate);
>-
>-                    if (!isCopyOnWrite(structure->indexingMode()))
>+                    switch (node->arrayMode().type()) {
>+                    case Array::Int32:
>+                    case Array::Contiguous:
>+                    case Array::Double:
>+                        if (structure->indexingMode() != (toIndexingShape(node->arrayMode().type()) | CopyOnWrite | IsArray))
>+                            return false;
>+                        break;
>+                    default:
>                         return false;
>+                    }
>+                    ASSERT(isCopyOnWrite(structure->indexingMode()));
> 
>                     JSImmutableButterfly* immutableButterfly = JSImmutableButterfly::fromButterfly(butterfly);
>                     if (index < immutableButterfly->length()) {
>diff --git a/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp b/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
>index ae2f15fd68403fcfb3ba9130886af983c1f944da..d2f83ac120d26f802b49cb17754a9f21ec50ef53 100644
>--- a/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGAbstractValue.cpp
>@@ -186,7 +186,7 @@ void AbstractValue::fixTypeForRepresentation(Graph& graph, NodeFlags representat
> {
>     if (representation == NodeResultDouble) {
>         if (m_value) {
>-            ASSERT(m_value.isNumber());
>+            DFG_ASSERT(graph, node, m_value.isNumber());
>             if (m_value.isInt32())
>                 m_value = jsDoubleNumber(m_value.asNumber());
>         }
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index b1f15c122d3f62d5e93e83a0263d50986a3f996c..3bb2256ba3b3f6a894031c87d8a2da7ca02a52d5 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,18 @@
>+2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
>+
>+        [JSC] AI should check the given constant's array type when folding GetByVal into constant
>+        https://bugs.webkit.org/show_bug.cgi?id=193413
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        This test is super flaky. It causes crash in r238109, but it does not crash with `--useConcurrentJIT=false`.
>+        It does not cause any crashes on the latest revision too. Basically, it highly depends on the timing, and
>+        without this patch, the root cause is not fixed yet. If GetLocal is turned into JSConstant in AI,
>+        but GetByVal does not have appropriate ArrayModes, JSC crashes.
>+
>+        * stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js: Added.
>+        (compareArray):
>+
> 2019-01-14  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [JSC] Do not use asArrayModes() with Structures because it discards TypedArray information
>diff --git a/JSTests/stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js b/JSTests/stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..be81502a468f3c7715ee5633db32147a0ddba0d9
>--- /dev/null
>+++ b/JSTests/stress/ai-should-perform-array-check-on-get-by-val-constant-folding.js
>@@ -0,0 +1,14 @@
>+//@ runDefault("--jitPolicyScale=0")
>+function compareArray(a, b) {
>+    if (b.length !== a.length) {
>+        return;
>+    }
>+    for (var i = 0; i < a.length; i++) {
>+        b[0];
>+    }
>+}
>+compareArray([], [0]);
>+compareArray([0, 'b'].copyWithin(), ['a', 0]);
>+compareArray([0], [1.1]);
>+runString('');
>+for (var i = 0; i < 1e6; ++i);

Actions: View | Formatted Diff | Diff

Attachments on bug 193413: 359098 | 359099