CheckIsConstant should not use BadCache exit kind
Created attachment 401764 [details] Patch
Created attachment 401765 [details] Patch
Comment on attachment 401765 [details] Patch Why?
Created attachment 401766 [details] Patch
Comment on attachment 401766 [details] Patch you've now broken places checking for the old exit kind
Comment on attachment 401766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401766&action=review > Source/JavaScriptCore/ChangeLog:10 > + The BadCache exit kind causes the OSR exit compilers to try to update ArrayProfiles. > + This is just incorrect for CheckIsConstant since the node's origin may not even > + have an ArrayProfile... why not just fix this by asking if an ArrayProfile exists?
(In reply to Saam Barati from comment #5) > Comment on attachment 401766 [details] > Patch > > you've now broken places checking for the old exit kind No one was looking for exit kind on for non-cells. The only place that uses this for non-cells is for-of and that handles it via falling into the generic case on exit.
(In reply to Saam Barati from comment #5) > Comment on attachment 401766 [details] > Patch > > you've now broken places checking for the old exit kind Keith pointed out that prior places were just for cells, and this is the non-cell path.
Comment on attachment 401766 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401766&action=review >> Source/JavaScriptCore/ChangeLog:10 >> + have an ArrayProfile... > > why not just fix this by asking if an ArrayProfile exists? We happen to have one for the origin in question so that wouldn't solve the problem.
Created attachment 401775 [details] Patch
Created attachment 401776 [details] Patch
Created attachment 401811 [details] Patch
Created attachment 401812 [details] Patch
rdar://64163856
Comment on attachment 401812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401812&action=review > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9676 > - speculationCheck(BadCache, regs, node->child1(), m_jit.branch64(JITCompiler::NotEqual, regs.gpr(), TrustedImm64(JSValue::encode(node->constant()->value())))); > + speculationCheck(BadConstantValue, regs, node->child1(), m_jit.branch64(JITCompiler::NotEqual, regs.gpr(), TrustedImm64(JSValue::encode(node->constant()->value())))); > #else > - speculationCheck(BadCache, regs, node->child1(), m_jit.branch32(JITCompiler::NotEqual, regs.tagGPR(), TrustedImm32(node->constant()->value().tag()))); > - speculationCheck(BadCache, regs, node->child1(), m_jit.branch32(JITCompiler::NotEqual, regs.payloadGPR(), TrustedImm32(node->constant()->value().payload()))); > + speculationCheck(BadConstantValue, regs, node->child1(), m_jit.branch32(JITCompiler::NotEqual, regs.tagGPR(), TrustedImm32(node->constant()->value().tag()))); > + speculationCheck(BadConstantValue, regs, node->child1(), m_jit.branch32(JITCompiler::NotEqual, regs.payloadGPR(), TrustedImm32(node->constant()->value().payload()))); One question. This changes BadCache -> BadConstantValue. So, we also need to check `hasExitSite(codeOrigin, BadCache)` things too. For example, Graph::canOptimizeStringObjectAccess has 1775 bool Graph::canOptimizeStringObjectAccess(const CodeOrigin& codeOrigin) 1776 { 1777 if (hasExitSite(codeOrigin, BadCache) || hasExitSite(codeOrigin, BadConstantCache)) 1778 return false; this code. PutById etc. has, 1847 case PutById: 1848 case PutByIdFlush: 1849 case PutByIdDirect: { 1850 if (node->child1()->shouldSpeculateCellOrOther() 1851 && !m_graph.hasExitSite(node->origin.semantic, BadType) 1852 && !m_graph.hasExitSite(node->origin.semantic, BadCache) 1853 && !m_graph.hasExitSite(node->origin.semantic, BadIndexingType) 1854 && !m_graph.hasExitSite(node->origin.semantic, ExoticObjectMode)) { this code. And there are many this type of codes. Can you ensure that they are correct after this change?
Comment on attachment 401812 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401812&action=review >> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:9676 >> + speculationCheck(BadConstantValue, regs, node->child1(), m_jit.branch32(JITCompiler::NotEqual, regs.payloadGPR(), TrustedImm32(node->constant()->value().payload()))); > > One question. This changes BadCache -> BadConstantValue. So, we also need to check `hasExitSite(codeOrigin, BadCache)` things too. > For example, Graph::canOptimizeStringObjectAccess has > > 1775 bool Graph::canOptimizeStringObjectAccess(const CodeOrigin& codeOrigin) > 1776 { > 1777 if (hasExitSite(codeOrigin, BadCache) || hasExitSite(codeOrigin, BadConstantCache)) > 1778 return false; > > this code. PutById etc. has, > > 1847 case PutById: > 1848 case PutByIdFlush: > 1849 case PutByIdDirect: { > 1850 if (node->child1()->shouldSpeculateCellOrOther() > 1851 && !m_graph.hasExitSite(node->origin.semantic, BadType) > 1852 && !m_graph.hasExitSite(node->origin.semantic, BadCache) > 1853 && !m_graph.hasExitSite(node->origin.semantic, BadIndexingType) > 1854 && !m_graph.hasExitSite(node->origin.semantic, ExoticObjectMode)) { > > this code. And there are many this type of codes. Can you ensure that they are correct after this change? The only code that used non-cell values for CheckIsConstant currently is the iterator bytecodes. And those nodes didn't actually look at the exit kind previously as we'll update our profiling post-OSR via the seen iteration mode profiles.
Committed r263054: <https://trac.webkit.org/changeset/263054> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401812 [details].