Bug 99287 - DFG should have some facility for recognizing redundant CheckArrays and Arrayifies
Summary: DFG should have some facility for recognizing redundant CheckArrays and Array...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 99371 98606
  Show dependency treegraph
 
Reported: 2012-10-14 20:21 PDT by Filip Pizlo
Modified: 2012-10-19 23:54 PDT (History)
8 users (show)

See Also:


Attachments
the patch (25.91 KB, patch)
2012-10-15 14:42 PDT, Filip Pizlo
buildbot: commit-queue-
Details | Formatted Diff | Diff
the patch (27.94 KB, patch)
2012-10-15 17:28 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (28.01 KB, patch)
2012-10-15 17:31 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (28.01 KB, patch)
2012-10-15 17:44 PDT, Filip Pizlo
mhahnenberg: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2012-10-14 20:21:44 PDT
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.
Comment 1 Filip Pizlo 2012-10-14 20:23:13 PDT
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.
Comment 2 Filip Pizlo 2012-10-15 14:42:48 PDT
Created attachment 168788 [details]
the patch

I think it's ready.  Still testing though, so not marking r? yet.
Comment 3 Eric Seidel (no email) 2012-10-15 15:53:15 PDT
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.
Comment 4 Build Bot 2012-10-15 17:10:27 PDT
Comment on attachment 168788 [details]
the patch

Attachment 168788 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14292928
Comment 5 Mark Hahnenberg 2012-10-15 17:16:21 PDT
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.
Comment 6 Filip Pizlo 2012-10-15 17:24:59 PDT
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'.
Comment 7 Filip Pizlo 2012-10-15 17:28:01 PDT
(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).
Comment 8 Filip Pizlo 2012-10-15 17:28:30 PDT
Created attachment 168816 [details]
the patch
Comment 9 Eric Seidel (no email) 2012-10-15 17:31:28 PDT
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.
Comment 10 Filip Pizlo 2012-10-15 17:31:51 PDT
Created attachment 168817 [details]
the patch

fix Windows build
Comment 11 Eric Seidel (no email) 2012-10-15 17:32:31 PDT
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.
Comment 12 Filip Pizlo 2012-10-15 17:44:02 PDT
Created attachment 168821 [details]
the patch

just rebasing
Comment 13 Mark Hahnenberg 2012-10-15 18:10:42 PDT
Comment on attachment 168821 [details]
the patch

r=me
Comment 14 Rik Cabanier 2012-10-15 21:10:10 PDT
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.
Comment 15 Build Bot 2012-10-16 03:40:16 PDT
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
Comment 16 Filip Pizlo 2012-10-19 23:54:22 PDT
Landed in http://trac.webkit.org/changeset/131982