Bug 175895 – [DFG] Add constant folding rule to convert CompareStrictEq(Untyped, Untyped [with non string cell constant]) to CompareEqPtr(Untyped)

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

...

[Yusuke Suzuki]

We start holding map bucket in js property. And we perform CompareSrictEq onto this.
Current DFG code looks nice. But we can optimize further by introducing SpecCellOther fixup and CellOtherUse.

[Yusuke Suzuki]

Or, we can say NotStringCellUse.

[Yusuke Suzuki]

Maybe, this is simpler.

[Yusuke Suzuki]

Created attachment 319134 [details]
Patch

[Saam Barati]

Comment on attachment 319134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319134&action=review

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:82
> +            if (!m_node->isBinaryUseKind(UntypedUse))

Why does this matter? It seems like your rewriting rule should work regardless? Or is this just because of the use kinds CompareEqPtr supports? If so, you can just convert this to a Check and then imagery the CompareEqPtr after that

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:88
> +

I think AI would be better at proving this than what you’re doing here. Maybe write such a rule in AI/constant folding?

[Yusuke Suzuki]

Comment on attachment 319134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319134&action=review

>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:82
>> +            if (!m_node->isBinaryUseKind(UntypedUse))
> 
> Why does this matter? It seems like your rewriting rule should work regardless? Or is this just because of the use kinds CompareEqPtr supports? If so, you can just convert this to a Check and then imagery the CompareEqPtr after that

In the other cases, we do not need to convert CompareStrictEq to CompareEqPtr. It is already fast and optimized enough in DFG speculative JIT.
And DFGSpeculativeJIT curretnly only supports CompareEqPtr(Untyped). I do not think CompareEqPtr(VariousType) is worth adding.

>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:88
>> +
> 
> I think AI would be better at proving this than what you’re doing here. Maybe write such a rule in AI/constant folding?

OK, I'll move this to AI/constant folding.

[Yusuke Suzuki]

Created attachment 319144 [details]
Patch

[Saam Barati]

Comment on attachment 319134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319134&action=review

>>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:82
>>> +            if (!m_node->isBinaryUseKind(UntypedUse))
>> 
>> Why does this matter? It seems like your rewriting rule should work regardless? Or is this just because of the use kinds CompareEqPtr supports? If so, you can just convert this to a Check and then imagery the CompareEqPtr after that
> 
> In the other cases, we do not need to convert CompareStrictEq to CompareEqPtr. It is already fast and optimized enough in DFG speculative JIT.
> And DFGSpeculativeJIT curretnly only supports CompareEqPtr(Untyped). I do not think CompareEqPtr(VariousType) is worth adding.

My point was that all you need to do to make this work is keep convert it to a CompareEqPtr and and add a Check right before the CompareEqPtr.
Also, it’s worth making sure this doesn’t need to be aware of document.all

[Yusuke Suzuki]

Comment on attachment 319134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319134&action=review

>>>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:82
>>>> +            if (!m_node->isBinaryUseKind(UntypedUse))
>>> 
>>> Why does this matter? It seems like your rewriting rule should work regardless? Or is this just because of the use kinds CompareEqPtr supports? If so, you can just convert this to a Check and then imagery the CompareEqPtr after that
>> 
>> In the other cases, we do not need to convert CompareStrictEq to CompareEqPtr. It is already fast and optimized enough in DFG speculative JIT.
>> And DFGSpeculativeJIT curretnly only supports CompareEqPtr(Untyped). I do not think CompareEqPtr(VariousType) is worth adding.
> 
> My point was that all you need to do to make this work is keep convert it to a CompareEqPtr and and add a Check right before the CompareEqPtr.
> Also, it’s worth making sure this doesn’t need to be aware of document.all

One concern is that it can hurt efficiency in 32bit environment. In 32bit, if we have CompareStrictEq(ObjectType, ObjectConstant), we can perform 32bit pointer comparison.
However, if we convert it to Check(Object) and CompareEqPtr(Untyped), it needs to perform 64bit comparison against JSValue. It is efficient in 64bit environment (since we can perform 64bit comparison), but it is not in 32bit environment.
I think the current conversion rule is good in efficiency. CompareStrictEq(Untyped, Untyped) anyway needs to perform JSValue comparison. If we can convert it to ComparePtrEq, it is always more efficient than CompareStrictEq.

One solution is adding CompareEqPtr(ObjectUse) etc. in 32bit environment (I don't think adding it to 64bit is good, since it can add an additional cell type check that is not necessary right now.) But I do not think it is worth adding.

I will add masquarade as undefined check too.

[Yusuke Suzuki]

Comment on attachment 319134 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319134&action=review

>>>>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:82
>>>>> +            if (!m_node->isBinaryUseKind(UntypedUse))
>>>> 
>>>> Why does this matter? It seems like your rewriting rule should work regardless? Or is this just because of the use kinds CompareEqPtr supports? If so, you can just convert this to a Check and then imagery the CompareEqPtr after that
>>> 
>>> In the other cases, we do not need to convert CompareStrictEq to CompareEqPtr. It is already fast and optimized enough in DFG speculative JIT.
>>> And DFGSpeculativeJIT curretnly only supports CompareEqPtr(Untyped). I do not think CompareEqPtr(VariousType) is worth adding.
>> 
>> My point was that all you need to do to make this work is keep convert it to a CompareEqPtr and and add a Check right before the CompareEqPtr.
>> Also, it’s worth making sure this doesn’t need to be aware of document.all
> 
> One concern is that it can hurt efficiency in 32bit environment. In 32bit, if we have CompareStrictEq(ObjectType, ObjectConstant), we can perform 32bit pointer comparison.
> However, if we convert it to Check(Object) and CompareEqPtr(Untyped), it needs to perform 64bit comparison against JSValue. It is efficient in 64bit environment (since we can perform 64bit comparison), but it is not in 32bit environment.
> I think the current conversion rule is good in efficiency. CompareStrictEq(Untyped, Untyped) anyway needs to perform JSValue comparison. If we can convert it to ComparePtrEq, it is always more efficient than CompareStrictEq.
> 
> One solution is adding CompareEqPtr(ObjectUse) etc. in 32bit environment (I don't think adding it to 64bit is good, since it can add an additional cell type check that is not necessary right now.) But I do not think it is worth adding.
> 
> I will add masquarade as undefined check too.

Ah, I do not need to add masquerade as undefined case since it is CompareStrictEq.

[Saam Barati]

Comment on attachment 319144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319144&action=review

r=me

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:158
> +                    if (isNonStringCellConstant(child1Constant)) {
> +                        node->convertToCompareEqPtr(m_graph.freezeStrong(child1Constant.asCell()), node->child2());
> +                        changed = true;
> +                    } else if (isNonStringCellConstant(child2Constant)) {
> +                        node->convertToCompareEqPtr(m_graph.freezeStrong(child2Constant.asCell()), node->child1());
> +                        changed = true;
> +                    }

When we do BigInt, we need to make sure we change this rule...

[Saam Barati]

(In reply to Yusuke Suzuki from comment #9)
> Comment on attachment 319134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319134&action=review
> 
> >>>> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:82
> >>>> +            if (!m_node->isBinaryUseKind(UntypedUse))
> >>> 
> >>> Why does this matter? It seems like your rewriting rule should work regardless? Or is this just because of the use kinds CompareEqPtr supports? If so, you can just convert this to a Check and then imagery the CompareEqPtr after that
> >> 
> >> In the other cases, we do not need to convert CompareStrictEq to CompareEqPtr. It is already fast and optimized enough in DFG speculative JIT.
> >> And DFGSpeculativeJIT curretnly only supports CompareEqPtr(Untyped). I do not think CompareEqPtr(VariousType) is worth adding.
> > 
> > My point was that all you need to do to make this work is keep convert it to a CompareEqPtr and and add a Check right before the CompareEqPtr.
> > Also, it’s worth making sure this doesn’t need to be aware of document.all
> 
> One concern is that it can hurt efficiency in 32bit environment. In 32bit,
> if we have CompareStrictEq(ObjectType, ObjectConstant), we can perform 32bit
> pointer comparison.
> However, if we convert it to Check(Object) and CompareEqPtr(Untyped), it
> needs to perform 64bit comparison against JSValue. It is efficient in 64bit
> environment (since we can perform 64bit comparison), but it is not in 32bit
> environment.
> I think the current conversion rule is good in efficiency.
> CompareStrictEq(Untyped, Untyped) anyway needs to perform JSValue
> comparison. If we can convert it to ComparePtrEq, it is always more
> efficient than CompareStrictEq.
> 
> One solution is adding CompareEqPtr(ObjectUse) etc. in 32bit environment (I
> don't think adding it to 64bit is good, since it can add an additional cell
> type check that is not necessary right now.) But I do not think it is worth
> adding.
> 
> I will add masquarade as undefined check too.

Yeah this makes sense. Also, you're totally right that ComapreStrictEq(ObjectUse:, some other use) is already fast.

[Yusuke Suzuki]

Comment on attachment 319144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319144&action=review

>> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:158
>> +                    }
> 
> When we do BigInt, we need to make sure we change this rule...

Yeah. Or we can take the similar approach to the current Symbol: interning BigInt cells by WeakGCMap to ensure the pointer comparison works.

[Saam Barati]

(In reply to Yusuke Suzuki from comment #13)
> Comment on attachment 319144 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319144&action=review
> 
> >> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:158
> >> +                    }
> > 
> > When we do BigInt, we need to make sure we change this rule...
> 
> Yeah. Or we can take the similar approach to the current Symbol: interning
> BigInt cells by WeakGCMap to ensure the pointer comparison works.

I don't think we'd want to intern these. That'll mean every math operation over them is doing some sort of hash-consing.

[Yusuke Suzuki]

Comment on attachment 319144 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319144&action=review

>>>> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:158
>>>> +                    }
>>> 
>>> When we do BigInt, we need to make sure we change this rule...
>> 
>> Yeah. Or we can take the similar approach to the current Symbol: interning BigInt cells by WeakGCMap to ensure the pointer comparison works.
> 
> I don't think we'd want to intern these. That'll mean every math operation over them is doing some sort of hash-consing.

OK, I'll add FIXME about it for the future BigInt integration :)

[Yusuke Suzuki]

Committed r221328: <http://trac.webkit.org/changeset/221328>

[Radar WebKit Bug Importer]

<rdar://problem/34148115>