Title: [238511] trunk
- Revision
- 238511
- Author
- [email protected]
- Date
- 2018-11-26 12:29:33 -0800 (Mon, 26 Nov 2018)
Log Message
InPlaceAbstractState::endBasicBlock rule for SetLocal should filter the value based on the flush format
https://bugs.webkit.org/show_bug.cgi?id=191956
<rdar://problem/45665806>
Reviewed by Yusuke Suzuki.
JSTests:
* stress/end-basic-block-set-local-should-filter-type.js: Added.
(bar):
(foo):
Source/_javascript_Core:
This is a similar bug to what Keith fixed in r232134. The issue is if we have
a program like this:
a: JSConstant(jsNumber(0))
b: SetLocal(Int32:@a, loc1, FlushedInt32)
c: ArrayifyToStructure(Cell:@a)
d: Jump(...)
At the point in the program right after the Jump, a GetLocal for loc1
would return whatever the ArrayifyToStructure resulting type is. This breaks
the invariant that a GetLocal must return a value that is a subtype of its
FlushFormat. InPlaceAbstractState::endBasicBlock will know if a SetLocal is
the final node touching a local slot. If so, it'll see if any nodes later
in the block may have refined the type of the value stored in that slot. If
so, endBasicBlock() further refines the type to ensure that any GetLocals
loading from the same slot will result in having this more refined type.
However, we must ensure that this logic only considers types within the
hierarchy of the variable access data's FlushFormat, otherwise, we may
break the invariant that a GetLocal's type is a subtype of its FlushFormat.
* dfg/DFGInPlaceAbstractState.cpp:
(JSC::DFG::InPlaceAbstractState::endBasicBlock):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (238510 => 238511)
--- trunk/JSTests/ChangeLog 2018-11-26 20:14:41 UTC (rev 238510)
+++ trunk/JSTests/ChangeLog 2018-11-26 20:29:33 UTC (rev 238511)
@@ -1,5 +1,17 @@
2018-11-26 Saam barati <[email protected]>
+ InPlaceAbstractState::endBasicBlock rule for SetLocal should filter the value based on the flush format
+ https://bugs.webkit.org/show_bug.cgi?id=191956
+ <rdar://problem/45665806>
+
+ Reviewed by Yusuke Suzuki.
+
+ * stress/end-basic-block-set-local-should-filter-type.js: Added.
+ (bar):
+ (foo):
+
+2018-11-26 Saam barati <[email protected]>
+
Object allocation sinking phase needs to iterate each scope offset instead of just iterating the symbol table's hashmap when handling an activation
https://bugs.webkit.org/show_bug.cgi?id=191958
<rdar://problem/46221877>
Added: trunk/JSTests/stress/end-basic-block-set-local-should-filter-type.js (0 => 238511)
--- trunk/JSTests/stress/end-basic-block-set-local-should-filter-type.js (rev 0)
+++ trunk/JSTests/stress/end-basic-block-set-local-should-filter-type.js 2018-11-26 20:29:33 UTC (rev 238511)
@@ -0,0 +1,16 @@
+function bar() {
+ let x = 0;
+ foo(0);
+ if (x) {
+ }
+}
+function foo(a) {
+ let x = a[0]
+ a[0] = 0;
+ return;
+ a
+}
+foo([0]);
+for (var i = 0; i < 10000; ++i) {
+ bar();
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (238510 => 238511)
--- trunk/Source/_javascript_Core/ChangeLog 2018-11-26 20:14:41 UTC (rev 238510)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-11-26 20:29:33 UTC (rev 238511)
@@ -1,5 +1,36 @@
2018-11-26 Saam barati <[email protected]>
+ InPlaceAbstractState::endBasicBlock rule for SetLocal should filter the value based on the flush format
+ https://bugs.webkit.org/show_bug.cgi?id=191956
+ <rdar://problem/45665806>
+
+ Reviewed by Yusuke Suzuki.
+
+ This is a similar bug to what Keith fixed in r232134. The issue is if we have
+ a program like this:
+
+ a: JSConstant(jsNumber(0))
+ b: SetLocal(Int32:@a, loc1, FlushedInt32)
+ c: ArrayifyToStructure(Cell:@a)
+ d: Jump(...)
+
+ At the point in the program right after the Jump, a GetLocal for loc1
+ would return whatever the ArrayifyToStructure resulting type is. This breaks
+ the invariant that a GetLocal must return a value that is a subtype of its
+ FlushFormat. InPlaceAbstractState::endBasicBlock will know if a SetLocal is
+ the final node touching a local slot. If so, it'll see if any nodes later
+ in the block may have refined the type of the value stored in that slot. If
+ so, endBasicBlock() further refines the type to ensure that any GetLocals
+ loading from the same slot will result in having this more refined type.
+ However, we must ensure that this logic only considers types within the
+ hierarchy of the variable access data's FlushFormat, otherwise, we may
+ break the invariant that a GetLocal's type is a subtype of its FlushFormat.
+
+ * dfg/DFGInPlaceAbstractState.cpp:
+ (JSC::DFG::InPlaceAbstractState::endBasicBlock):
+
+2018-11-26 Saam barati <[email protected]>
+
Object allocation sinking phase needs to iterate each scope offset instead of just iterating the symbol table's hashmap when handling an activation
https://bugs.webkit.org/show_bug.cgi?id=191958
<rdar://problem/46221877>
Modified: trunk/Source/_javascript_Core/dfg/DFGInPlaceAbstractState.cpp (238510 => 238511)
--- trunk/Source/_javascript_Core/dfg/DFGInPlaceAbstractState.cpp 2018-11-26 20:14:41 UTC (rev 238510)
+++ trunk/Source/_javascript_Core/dfg/DFGInPlaceAbstractState.cpp 2018-11-26 20:29:33 UTC (rev 238511)
@@ -265,8 +265,23 @@
}
case SetLocal: {
// The block sets the variable, and potentially refines it, both
- // before and after setting it.
- destination = forNode(node->child1());
+ // before and after setting it. Since the SetLocal already did
+ // a type check based on the flush format's type, we're only interested
+ // in refinements within that type hierarchy. Otherwise, we may end up
+ // saying that any GetLocals reachable from this basic block load something
+ // outside of that hierarchy, e.g:
+ //
+ // a: JSConstant(jsNumber(0))
+ // b: SetLocal(Int32:@a, loc1, FlushedInt32)
+ // c: ArrayifyToStructure(Cell:@a)
+ // d: Jump(...)
+ //
+ // In this example, we can't trust whatever type ArrayifyToStructure sets
+ // @a to. We're only interested in the subset of that type that intersects
+ // with Int32.
+ AbstractValue value = forNode(node->child1());
+ value.filter(typeFilterFor(node->variableAccessData()->flushFormat()));
+ destination = value;
break;
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
