Let's consider the following code. 32:<!0:-> GetLocal(Check:Untyped:@1, JS|MustGen|UseAsOther, StringIdent|Other, arg1(B~/FlushedJSValue), R:Stack(6), bc#15, ExitValid) predicting StringIdent|Other 33:<!0:-> PutByOffset(KnownCell:@29, KnownCell:@29, Check:Untyped:@32, MustGen, id0{y}, 0, inferredType = Bottom, W:NamedProperties(0), ClobbersExit, bc#15, ExitValid) 47:< 1:-> GetByOffset(KnownCell:@29, KnownCell:@29, JS|UseAsOther, StringIdent, id0{y}, 0, inferredType = String, R:NamedProperties(0), Exits, bc#39, ExitValid) predicting StringIdent ... 49:< 1:-> SetLocal(Cell:@47, IsFlushed, loc11(Q<StringIdent>/FlushedCell), W:Stack(-12), bc#39, exit: bc#44, ExitValid) predicting StringIdent ... 87:<!0:-> GetLocal(Check:Untyped:@154, JS|MustGen|UseAsOther, StringIdent, loc11(Q<StringIdent>/FlushedCell), R:Stack(-12), bc#16, ExitValid) predicting StringIdent ... 173:<!0:-> Check(String:@87, MustGen, bc#16, ExitValid) ... 89:<!0:-> GetByVal(KnownCell:@87, Int32:@88, Check:Untyped:@174, JS|MustGen|VarArgs|PureInt, StringIdent, String+NonArray+InBounds+AsIs+Read, Exits, bc#16, ExitValid) predicting StringIdent In the first AI phase, PutByOffset and GetByOffset prove that given @32 value is always String by using inferred type. So, @47 becomes String. And @173 will be removed since @87 is now proven as String (SetLocal and GetLocal). But, after that, we perform LocalCSE. At that time, @47 GetByOffset is converted to @32 by using heap location information. @87 GetLocal is converted to @32 by using stack location information. And then, after the conversion, we will get the following code. 32:<!0:-> GetLocal(Check:Untyped:@1, JS|MustGen|UseAsOther, StringIdent|Other, arg1(B~/FlushedJSValue), R:Stack(6), bc#15, ExitValid) predicting StringIdent|Other ... 89:<!0:-> GetByVal(KnownCell:@32, Int32:@79, Check:Untyped:@174, JS|MustGen|VarArgs|PureInt, StringIdent, String+NonArray+InBounds+AsIs+Read, Exits, bc#16, ExitValid) predicting StringIdent Then, the problem is that, GetLocal's AbstractValue is no longer proven as String. Our CSE weaken the AbstractValue, so that this is now Cell. Then, DFGSpeculativeJIT.cpp assertion hits in GetByVal compiling code. ArrayMode(Array::String, Array::Read).alreadyChecked(m_jit.graph(), node, m_state.forNode(m_graph.child(node, 0)))
<rdar://problem/45581249>
The issue is that 1. GetByOffset in AI does not include result type information to HeapLocation (in Clobberize) 2. CSE performs substitution between GetByOffset with a fancy different node that has weaker output type constraint. In this case, GetLocal through PutByOffset. 3. SpeculativeJIT hits assertion since the type constraint is weaken. Discussed with Saam, the correct fix is including inferred type information into PutByOffset / GetByOffset's HeapLocation. The issue itself would be gone once inferred types are removed in bug 190906. But basically the root cause of this issue is that some nodes do not include its output type information in clobberizing rule and so that CSE will accidentally replace the node with the node which has different output type constraints (if it is weaker, then, we have a bad time). I'll review nodes in AI to figure out the potential problems further.
(In reply to Yusuke Suzuki from comment #2) > The issue is that > > 1. GetByOffset in AI does not include result type information to > HeapLocation (in Clobberize) > 2. CSE performs substitution between GetByOffset with a fancy different node > that has weaker output type constraint. In this case, GetLocal through > PutByOffset. > 3. SpeculativeJIT hits assertion since the type constraint is weaken. > > Discussed with Saam, the correct fix is including inferred type information > into PutByOffset / GetByOffset's HeapLocation. > The issue itself would be gone once inferred types are removed in bug 190906. > But basically the root cause of this issue is that some nodes do not include > its output type information in clobberizing rule and so that CSE will > accidentally replace the node with the node which has different output type > constraints (if it is weaker, then, we have a bad time). > > I'll review nodes in AI to figure out the potential problems further. MultiGetByOffset seems a bit dangerous? Investigating...
After discussing with Saam, the above problem exists, but we can fix this issue by using KnownStringUse. I'll upload a patch with this fix.
Created attachment 359220 [details] Patch
Comment on attachment 359220 [details] Patch GetArrayLength is expecting KnownCellUse. And it is OK. But in FTL, we emit GetArrayLength from GetByVal to lower CheckInBounds. At that time, it becomes GetArrayLength(KnownStringUse). We should support GetArrayLength(KnownStringUse) too.
(In reply to Yusuke Suzuki from comment #6) > Comment on attachment 359220 [details] > Patch > > GetArrayLength is expecting KnownCellUse. And it is OK. But in FTL, we emit > GetArrayLength from GetByVal to lower CheckInBounds. At that time, it > becomes GetArrayLength(KnownStringUse). We should support > GetArrayLength(KnownStringUse) too. Or, just simply converting Edge(KnownStringUse) => Edge(KnownCellUse). It's simpler.
(In reply to Yusuke Suzuki from comment #7) > (In reply to Yusuke Suzuki from comment #6) > > Comment on attachment 359220 [details] > > Patch > > > > GetArrayLength is expecting KnownCellUse. And it is OK. But in FTL, we emit > > GetArrayLength from GetByVal to lower CheckInBounds. At that time, it > > becomes GetArrayLength(KnownStringUse). We should support > > GetArrayLength(KnownStringUse) too. > > Or, just simply converting Edge(KnownStringUse) => Edge(KnownCellUse). It's > simpler. No. We do not need to do it since we do not lower GetByVal(Array::String) => CheckInBounds + GetByVal. Adding an assertion now.
Created attachment 359226 [details] Patch
Created attachment 359227 [details] Patch
Comment on attachment 359227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359227&action=review > Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:129 > + RELEASE_ASSERT_NOT_REACHED(); DFG_ASSERT please.
Comment on attachment 359227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359227&action=review > Source/JavaScriptCore/ChangeLog:10 > + After the first run removes Check(String), it would happen that AI starts saying the tyep of 1st child is not String. typo: tyep => type. > Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:128 > + // When we need supporting this, it requires an additional code since base's useKind is KnownStringUse. Nit: This should probably be: "When we need to support this, it will require additional code since base's useKind is KnownStringUse."
r=me too.
Comment on attachment 359227 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=359227&action=review Thank you! >> Source/JavaScriptCore/ChangeLog:10 >> + After the first run removes Check(String), it would happen that AI starts saying the tyep of 1st child is not String. > > typo: tyep => type. Oops, thanks. Fixed. >> Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:128 >> + // When we need supporting this, it requires an additional code since base's useKind is KnownStringUse. > > Nit: This should probably be: "When we need to support this, it will require additional code since base's useKind is KnownStringUse." Nice. Fixed. >> Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp:129 >> + RELEASE_ASSERT_NOT_REACHED(); > > DFG_ASSERT please. Fixed. Change this to DFG_CRASH().
Fixed in https://trac.webkit.org/changeset/240024/webkit