Bug 99287 – DFG should have some facility for recognizing redundant CheckArrays and Arrayifies

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Currently if we emit those things, then they don't get killed even if they're obviously redundant, like:

a[i] + a[i + 1];

If i is predicted int, and a is predicted to be almost any sensible array mode, then we should only be checking a's type once.  That works fine if we also predict a's structure.  But if we don't have a structure prediction, and just have an array mode prediction, then this all falls apart, and we end up with two CheckArray nodes.

[Filip Pizlo]

The easiest way to do this is to teach the CSE about these things.  But to be able to do things like CheckArray/Arrayify hoisting, we really need the CFA to know about array mode.

[Filip Pizlo]

Created attachment 168788 [details]
the patch

I think it's ready.  Still testing though, so not marking r? yet.

[Eric Seidel (no email)]

Attachment 168788 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/bytecode/ArrayProfile.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:54:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 168788 [details]
the patch

Attachment 168788 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14292928

[Mark Hahnenberg]

Comment on attachment 168788 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168788&action=review

> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:201
> +        if (arrayModesAlreadyChecked(value.m_arrayModes, asArrayModes(NonArrayWithArrayStorage) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))

I'm confused by this. Is this supposed to return true like the others? If not, the return should probably be indented. Also, is "|" set intersection or union? It looks like all of these are SlowPutArrayStorage, but you check for anything with ArrayStorage? Sorry if I'm being dense.

[Filip Pizlo]

Comment on attachment 168788 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168788&action=review

>> Source/JavaScriptCore/dfg/DFGArrayMode.cpp:201
>> +        if (arrayModesAlreadyChecked(value.m_arrayModes, asArrayModes(NonArrayWithArrayStorage) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))
> 
> I'm confused by this. Is this supposed to return true like the others? If not, the return should probably be indented. Also, is "|" set intersection or union? It looks like all of these are SlowPutArrayStorage, but you check for anything with ArrayStorage? Sorry if I'm being dense.

Oh, good catch.  This should be 'return true'.

The expression "asArrayModes(NonArrayWithArrayStorage) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)" denotes the set of all objects that have array storage or slow put array storage.

This is in contrast to "asArrayModes(NonArrayWithArrayStorage) | asArrayModes(ArrayWithArrayStorage)" which denotes the set of all objects that have array storage.

Or "asArrayModes(NonArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage)" which denotes the set of all non-array objects that have array storage or slow put array storage.

And so on.

I'll add the 'return true'.

[Filip Pizlo]

(In reply to comment #5)
> (From update of attachment 168788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168788&action=review
> 
> > Source/JavaScriptCore/dfg/DFGArrayMode.cpp:201
> > +        if (arrayModesAlreadyChecked(value.m_arrayModes, asArrayModes(NonArrayWithArrayStorage) | asArrayModes(ArrayWithArrayStorage) | asArrayModes(NonArrayWithSlowPutArrayStorage) | asArrayModes(ArrayWithSlowPutArrayStorage)))
> 
> I'm confused by this. Is this supposed to return true like the others? If not, the return should probably be indented. Also, is "|" set intersection or union? It looks like all of these are SlowPutArrayStorage, but you check for anything with ArrayStorage? Sorry if I'm being dense.

Oh, sorry - I think I know what you're asking.  The point is that if we're compiling an array access in slow put mode, then we're willing to accept any kind of array storage object (array or not, slow put or not).

[Filip Pizlo]

Created attachment 168816 [details]
the patch

[Eric Seidel (no email)]

Attachment 168816 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/bytecode/ArrayProfile.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:54:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 168817 [details]
the patch

fix Windows build

[Eric Seidel (no email)]

Attachment 168817 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
Source/JavaScriptCore/bytecode/ArrayProfile.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:54:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 168821 [details]
the patch

just rebasing

[Mark Hahnenberg]

Comment on attachment 168821 [details]
the patch

r=me

[Rik Cabanier]

Attachment 168821 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1
WARNING: Patch's size is 28 kbytes.
Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time.
Source/JavaScriptCore/bytecode/ArrayProfile.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:49:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:53:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:54:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/bytecode/ArrayProfile.h:55:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 6 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 168821 [details]
the patch

Attachment 168821 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14385147

New failing tests:
inspector/elements/insert-node.html

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/131982