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)

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yusuke Suzuki]

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)))

[Yusuke Suzuki]

<rdar://problem/45581249>

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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...

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Created attachment 359220 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

(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.

[Yusuke Suzuki]

Created attachment 359226 [details]
Patch

[Yusuke Suzuki]

Created attachment 359227 [details]
Patch

[Saam Barati]

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.

[Keith Miller]

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."

[Keith Miller]

r=me too.

[Yusuke Suzuki]

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().

[Yusuke Suzuki]

Fixed in https://trac.webkit.org/changeset/240024/webkit