RESOLVED FIXED 213141
CheckIsConstant should not use BadCache exit kind
https://bugs.webkit.org/show_bug.cgi?id=213141
Summary CheckIsConstant should not use BadCache exit kind
Keith Miller
Reported 2020-06-12 11:57:36 PDT
CheckIsConstant should not use BadCache exit kind
Attachments
Patch (3.56 KB, patch)
2020-06-12 12:32 PDT, Keith Miller
no flags
Patch (3.55 KB, patch)
2020-06-12 12:32 PDT, Keith Miller
no flags
Patch (4.88 KB, patch)
2020-06-12 12:49 PDT, Keith Miller
no flags
Patch (25.13 KB, patch)
2020-06-12 13:51 PDT, Keith Miller
no flags
Patch (25.50 KB, patch)
2020-06-12 13:58 PDT, Keith Miller
no flags
Patch (25.76 KB, patch)
2020-06-12 17:35 PDT, Keith Miller
no flags
Patch (25.77 KB, patch)
2020-06-12 17:36 PDT, Keith Miller
no flags
Keith Miller
Comment 1 2020-06-12 12:32:10 PDT
Keith Miller
Comment 2 2020-06-12 12:32:41 PDT
Saam Barati
Comment 3 2020-06-12 12:35:16 PDT
Comment on attachment 401765 [details] Patch Why?
Keith Miller
Comment 4 2020-06-12 12:49:49 PDT
Saam Barati
Comment 5 2020-06-12 12:53:49 PDT
Comment on attachment 401766 [details] Patch you've now broken places checking for the old exit kind
Saam Barati
Comment 6 2020-06-12 12:55:25 PDT
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?
Keith Miller
Comment 7 2020-06-12 12:56:16 PDT
(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.
Saam Barati
Comment 8 2020-06-12 12:56:41 PDT
(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.
Keith Miller
Comment 9 2020-06-12 12:57:03 PDT
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.
Keith Miller
Comment 10 2020-06-12 13:51:15 PDT
Keith Miller
Comment 11 2020-06-12 13:58:38 PDT
Keith Miller
Comment 12 2020-06-12 17:35:16 PDT
Keith Miller
Comment 13 2020-06-12 17:36:14 PDT
Keith Miller
Comment 14 2020-06-15 12:22:39 PDT
Yusuke Suzuki
Comment 15 2020-06-15 12:31:34 PDT
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?
Keith Miller
Comment 16 2020-06-15 12:35:28 PDT
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.
EWS
Comment 17 2020-06-15 12:50:07 PDT
Committed r263054: <https://trac.webkit.org/changeset/263054> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401812 [details].
Note You need to log in before you can comment on or make changes to this bug.