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.
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.
Created attachment 168788 [details] the patch I think it's ready. Still testing though, so not marking r? yet.
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 on attachment 168788 [details] the patch Attachment 168788 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14292928
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 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'.
(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).
Created attachment 168816 [details] the patch
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.
Created attachment 168817 [details] the patch fix Windows build
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.
Created attachment 168821 [details] the patch just rebasing
Comment on attachment 168821 [details] the patch r=me
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 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
Landed in http://trac.webkit.org/changeset/131982