Bug 213141 – CheckIsConstant should not use BadCache exit kind

• Component: New Bugs
• Status: RESOLVED FIXED

[Keith Miller]

CheckIsConstant should not use BadCache exit kind

[Keith Miller]

Created attachment 401764 [details]
Patch

[Keith Miller]

Created attachment 401765 [details]
Patch

[Saam Barati]

Comment on attachment 401765 [details]
Patch

Why?

[Keith Miller]

Created attachment 401766 [details]
Patch

[Saam Barati]

Comment on attachment 401766 [details]
Patch

you've now broken places checking for the old exit kind

[Saam Barati]

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]

(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]

(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 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]

Created attachment 401775 [details]
Patch

[Keith Miller]

Created attachment 401776 [details]
Patch

[Keith Miller]

Created attachment 401811 [details]
Patch

[Keith Miller]

Created attachment 401812 [details]
Patch

[Keith Miller]

rdar://64163856

[Yusuke Suzuki]

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 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]

Committed r263054: <https://trac.webkit.org/changeset/263054>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401812 [details].