...
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.
Or, we can say NotStringCellUse.
Maybe, this is simpler.
Created attachment 319134 [details] Patch
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?
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.
Created attachment 319144 [details] Patch
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
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.
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.
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...
(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.
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.
(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.
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 :)
Committed r221328: <http://trac.webkit.org/changeset/221328>
<rdar://problem/34148115>