Bug 193438 - [JSC] Use KnownStringUse for GetByVal(Array::String) since AI would offer wider type information and offer non-string type after removing Check(String)
Summary: [JSC] Use KnownStringUse for GetByVal(Array::String) since AI would offer wid...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-01-15 00:08 PST by Yusuke Suzuki
Modified: 2019-01-15 18:18 PST (History)
6 users (show)

See Also:


Attachments
Patch (5.42 KB, patch)
2019-01-15 16:26 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (7.39 KB, patch)
2019-01-15 16:51 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (7.64 KB, patch)
2019-01-15 16:54 PST, Yusuke Suzuki
keith_miller: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-01-15 00:08:08 PST
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)))
Comment 1 Yusuke Suzuki 2019-01-15 00:09:06 PST
<rdar://problem/45581249>
Comment 2 Yusuke Suzuki 2019-01-15 12:48:50 PST
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.
Comment 3 Yusuke Suzuki 2019-01-15 14:18:54 PST
(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...
Comment 4 Yusuke Suzuki 2019-01-15 16:19:26 PST
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.
Comment 5 Yusuke Suzuki 2019-01-15 16:26:28 PST
Created attachment 359220 [details]
Patch
Comment 6 Yusuke Suzuki 2019-01-15 16:31:42 PST
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.
Comment 7 Yusuke Suzuki 2019-01-15 16:34:30 PST
(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.
Comment 8 Yusuke Suzuki 2019-01-15 16:37:33 PST
(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.
Comment 9 Yusuke Suzuki 2019-01-15 16:51:31 PST
Created attachment 359226 [details]
Patch
Comment 10 Yusuke Suzuki 2019-01-15 16:54:17 PST
Created attachment 359227 [details]
Patch
Comment 11 Saam Barati 2019-01-15 17:01:39 PST
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 12 Keith Miller 2019-01-15 17:03:02 PST
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."
Comment 13 Keith Miller 2019-01-15 17:03:29 PST
r=me too.
Comment 14 Yusuke Suzuki 2019-01-15 17:15:47 PST
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().
Comment 15 Yusuke Suzuki 2019-01-15 18:18:49 PST
Fixed in https://trac.webkit.org/changeset/240024/webkit