Title: [234090] trunk
- Revision
- 234090
- Author
- [email protected]
- Date
- 2018-07-22 12:24:34 -0700 (Sun, 22 Jul 2018)
Log Message
[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.
JSTests:
* stress/in-miss-variant-merge.js: Added.
(shouldBe):
(test):
* stress/miss-variant-merge.js: Added.
(shouldBe):
(test):
Source/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (234089 => 234090)
--- trunk/JSTests/ChangeLog 2018-07-22 16:54:38 UTC (rev 234089)
+++ trunk/JSTests/ChangeLog 2018-07-22 19:24:34 UTC (rev 234090)
@@ -1,5 +1,19 @@
2018-07-22 Yusuke Suzuki <[email protected]>
+ [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 <[email protected]>
+
[DFG] Fold GetByVal if the indexed value is non configurable and non writable
https://bugs.webkit.org/show_bug.cgi?id=186462
Added: trunk/JSTests/stress/in-miss-variant-merge.js (0 => 234090)
--- trunk/JSTests/stress/in-miss-variant-merge.js (rev 0)
+++ trunk/JSTests/stress/in-miss-variant-merge.js 2018-07-22 19:24:34 UTC (rev 234090)
@@ -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);
+}
Added: trunk/JSTests/stress/miss-variant-merge.js (0 => 234090)
--- trunk/JSTests/stress/miss-variant-merge.js (rev 0)
+++ trunk/JSTests/stress/miss-variant-merge.js 2018-07-22 19:24:34 UTC (rev 234090)
@@ -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);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (234089 => 234090)
--- trunk/Source/_javascript_Core/ChangeLog 2018-07-22 16:54:38 UTC (rev 234089)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-07-22 19:24:34 UTC (rev 234090)
@@ -1,5 +1,26 @@
2018-07-22 Yusuke Suzuki <[email protected]>
+ [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 <[email protected]>
+
[DFG] Fold GetByVal if the indexed value is non configurable and non writable
https://bugs.webkit.org/show_bug.cgi?id=186462
Modified: trunk/Source/_javascript_Core/bytecode/GetByIdVariant.cpp (234089 => 234090)
--- trunk/Source/_javascript_Core/bytecode/GetByIdVariant.cpp 2018-07-22 16:54:38 UTC (rev 234089)
+++ trunk/Source/_javascript_Core/bytecode/GetByIdVariant.cpp 2018-07-22 19:24:34 UTC (rev 234090)
@@ -128,8 +128,11 @@
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;
Modified: trunk/Source/_javascript_Core/bytecode/InByIdVariant.cpp (234089 => 234090)
--- trunk/Source/_javascript_Core/bytecode/InByIdVariant.cpp 2018-07-22 16:54:38 UTC (rev 234089)
+++ trunk/Source/_javascript_Core/bytecode/InByIdVariant.cpp 2018-07-22 19:24:34 UTC (rev 234090)
@@ -54,8 +54,11 @@
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;
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
