Bug 99287

Summary:

DFG should have some facility for recognizing redundant CheckArrays and Arrayifies

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, cabanier, eric, ggaren, mark.lam, mhahnenberg, msaboff, oliver

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

99371, 98606

Attachments:

Description	Flags
the patch	buildbot: commit-queue-
the patch	none
the patch	none
the patch	mhahnenberg: review+, buildbot: commit-queue-

Filip Pizlo

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Filip Pizlo

Comment 1 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.

Filip Pizlo

Comment 2 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.

Eric Seidel (no email)

Comment 3 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.

Build Bot

Comment 4 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

Mark Hahnenberg

Comment 5 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.

Filip Pizlo

Comment 6 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'.

Filip Pizlo

Comment 7 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).

Filip Pizlo

Comment 8 2012-10-15 17:28:30 PDT

Created attachment 168816 [details] the patch

Eric Seidel (no email)

Comment 9 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.

Filip Pizlo

Comment 10 2012-10-15 17:31:51 PDT

Created attachment 168817 [details] the patch fix Windows build

Eric Seidel (no email)

Comment 11 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.

Filip Pizlo

Comment 12 2012-10-15 17:44:02 PDT

Created attachment 168821 [details] the patch just rebasing

Mark Hahnenberg

Comment 13 2012-10-15 18:10:42 PDT

Comment on attachment 168821 [details] the patch r=me

Rik Cabanier

Comment 14 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.

Build Bot

Comment 15 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

Filip Pizlo

Comment 16 2012-10-19 23:54:22 PDT

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

Note You need to log in before you can comment on or make changes to this bug.