[JSC] GetByIdVariant and InByIdVariant do not need slot base if they are not "hit...
authorutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 22 Jul 2018 19:24:34 +0000 (19:24 +0000)
committerutatane.tea@gmail.com <utatane.tea@gmail.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 22 Jul 2018 19:24:34 +0000 (19:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=187891

Reviewed by Saam Barati.

JSTests:

* stress/in-miss-variant-merge.js: Added.
(shouldBe):
(test):
* stress/miss-variant-merge.js: Added.
(shouldBe):
(test):

Source/JavaScriptCore:

When merging GetByIdVariant and InByIdVariant, we accidentally make merging failed if
two variants are mergeable but they have "Miss" status. We make merging failed if
the merged OPCSet says hasOneSlotBaseCondition() is false. But it is only reasonable
if the variant has "Hit" status. This bug is revealed when we introduce CreateThis in FTL,
which patch have more chances to merge variants.

This patch fixes this issue by checking `!isPropertyUnset()` / `isHit()`. PutByIdVariant
is not related since it does not use this check in Transition case.

* bytecode/GetByIdVariant.cpp:
(JSC::GetByIdVariant::attemptToMerge):
* bytecode/InByIdVariant.cpp:
(JSC::InByIdVariant::attemptToMerge):

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

JSTests/ChangeLog
JSTests/stress/in-miss-variant-merge.js [new file with mode: 0644]
JSTests/stress/miss-variant-merge.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/GetByIdVariant.cpp
Source/JavaScriptCore/bytecode/InByIdVariant.cpp

index dcda352..af77bfe 100644 (file)
@@ -1,5 +1,19 @@
 2018-07-22  Yusuke Suzuki  <utatane.tea@gmail.com>
 
+        [JSC] GetByIdVariant and InByIdVariant do not need slot base if they are not "hit" variants
+        https://bugs.webkit.org/show_bug.cgi?id=187891
+
+        Reviewed by Saam Barati.
+
+        * stress/in-miss-variant-merge.js: Added.
+        (shouldBe):
+        (test):
+        * stress/miss-variant-merge.js: Added.
+        (shouldBe):
+        (test):
+
+2018-07-22  Yusuke Suzuki  <utatane.tea@gmail.com>
+
         [DFG] Fold GetByVal if the indexed value is non configurable and non writable
         https://bugs.webkit.org/show_bug.cgi?id=186462
 
diff --git a/JSTests/stress/in-miss-variant-merge.js b/JSTests/stress/in-miss-variant-merge.js
new file mode 100644 (file)
index 0000000..552ae59
--- /dev/null
@@ -0,0 +1,21 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+noInline(shouldBe);
+
+function test(object)
+{
+    return 'return' in object;
+}
+noInline(test);
+
+var object1 = {};
+var object2 = { hello: 42 };
+for (var i = 0; i < 10; ++i) {
+    shouldBe(test(object1), false);
+}
+for (var i = 0; i < 1e6; ++i) {
+    shouldBe(test(object1), false);
+    shouldBe(test(object2), false);
+}
diff --git a/JSTests/stress/miss-variant-merge.js b/JSTests/stress/miss-variant-merge.js
new file mode 100644 (file)
index 0000000..258c441
--- /dev/null
@@ -0,0 +1,21 @@
+function shouldBe(actual, expected) {
+    if (actual !== expected)
+        throw new Error('bad value: ' + actual);
+}
+noInline(shouldBe);
+
+function test(object)
+{
+    return object.return;
+}
+noInline(test);
+
+var object1 = {};
+var object2 = { hello: 42 };
+for (var i = 0; i < 10; ++i) {
+    shouldBe(test(object1), undefined);
+}
+for (var i = 0; i < 1e6; ++i) {
+    shouldBe(test(object1), undefined);
+    shouldBe(test(object2), undefined);
+}
index 7a7797b..ef79ffd 100644 (file)
@@ -1,5 +1,26 @@
 2018-07-22  Yusuke Suzuki  <utatane.tea@gmail.com>
 
+        [JSC] GetByIdVariant and InByIdVariant do not need slot base if they are not "hit" variants
+        https://bugs.webkit.org/show_bug.cgi?id=187891
+
+        Reviewed by Saam Barati.
+
+        When merging GetByIdVariant and InByIdVariant, we accidentally make merging failed if
+        two variants are mergeable but they have "Miss" status. We make merging failed if
+        the merged OPCSet says hasOneSlotBaseCondition() is false. But it is only reasonable
+        if the variant has "Hit" status. This bug is revealed when we introduce CreateThis in FTL,
+        which patch have more chances to merge variants.
+
+        This patch fixes this issue by checking `!isPropertyUnset()` / `isHit()`. PutByIdVariant
+        is not related since it does not use this check in Transition case.
+
+        * bytecode/GetByIdVariant.cpp:
+        (JSC::GetByIdVariant::attemptToMerge):
+        * bytecode/InByIdVariant.cpp:
+        (JSC::InByIdVariant::attemptToMerge):
+
+2018-07-22  Yusuke Suzuki  <utatane.tea@gmail.com>
+
         [DFG] Fold GetByVal if the indexed value is non configurable and non writable
         https://bugs.webkit.org/show_bug.cgi?id=186462
 
index e437140..8ae40e3 100644 (file)
@@ -128,7 +128,10 @@ bool GetByIdVariant::attemptToMerge(const GetByIdVariant& other)
     ObjectPropertyConditionSet mergedConditionSet;
     if (!m_conditionSet.isEmpty()) {
         mergedConditionSet = m_conditionSet.mergedWith(other.m_conditionSet);
-        if (!mergedConditionSet.isValid() || !mergedConditionSet.hasOneSlotBaseCondition())
+        if (!mergedConditionSet.isValid())
+            return false;
+        // If this is a hit variant, one slot base should exist. If this is not a hit variant, the slot base is not necessary.
+        if (!isPropertyUnset() && !mergedConditionSet.hasOneSlotBaseCondition())
             return false;
     }
     m_conditionSet = mergedConditionSet;
index a7c3cd6..e2e8fa6 100644 (file)
@@ -54,7 +54,10 @@ bool InByIdVariant::attemptToMerge(const InByIdVariant& other)
     ObjectPropertyConditionSet mergedConditionSet;
     if (!m_conditionSet.isEmpty()) {
         mergedConditionSet = m_conditionSet.mergedWith(other.m_conditionSet);
-        if (!mergedConditionSet.isValid() || !mergedConditionSet.hasOneSlotBaseCondition())
+        if (!mergedConditionSet.isValid())
+            return false;
+        // If this is a hit variant, one slot base should exist. If this is not a hit variant, the slot base is not necessary.
+        if (isHit() && !mergedConditionSet.hasOneSlotBaseCondition())
             return false;
     }
     m_conditionSet = mergedConditionSet;