WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(3.55 KB, patch)
2020-06-12 12:32 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(4.88 KB, patch)
2020-06-12 12:49 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(25.13 KB, patch)
2020-06-12 13:51 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(25.50 KB, patch)
2020-06-12 13:58 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(25.76 KB, patch)
2020-06-12 17:35 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Patch
(25.77 KB, patch)
2020-06-12 17:36 PDT
,
Keith Miller
no flags
Details
Formatted Diff
Diff
Show Obsolete
(6)
View All
Add attachment
proposed patch, testcase, etc.
Keith Miller
Comment 1
2020-06-12 12:32:10 PDT
Created
attachment 401764
[details]
Patch
Keith Miller
Comment 2
2020-06-12 12:32:41 PDT
Created
attachment 401765
[details]
Patch
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
Created
attachment 401766
[details]
Patch
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
Created
attachment 401775
[details]
Patch
Keith Miller
Comment 11
2020-06-12 13:58:38 PDT
Created
attachment 401776
[details]
Patch
Keith Miller
Comment 12
2020-06-12 17:35:16 PDT
Created
attachment 401811
[details]
Patch
Keith Miller
Comment 13
2020-06-12 17:36:14 PDT
Created
attachment 401812
[details]
Patch
Keith Miller
Comment 14
2020-06-15 12:22:39 PDT
rdar://64163856
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.
Top of Page
Format For Printing
XML
Clone This Bug