Bug 94448

Summary: Array accesses should remember what kind of array they are predicted to access
Product: WebKit Reporter: Filip Pizlo <fpizlo>
Component: JavaScriptCoreAssignee: Filip Pizlo <fpizlo>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, ggaren, mhahnenberg, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 91933    
Attachments:
Description Flags
work in progress
none
work in progress
none
work in progress
none
almost there
none
getting there
none
it runs simple things
none
more
none
seems to work
none
the patch barraclough: review+

Description Filip Pizlo 2012-08-20 00:24:00 PDT 
It used to be that array accesses would know what kind of array they're accessing based on the predictions of their children. But that's no longer the case, since part of the decision process of what kind of array access to compile comes from the ArrayProfile. Hence, array accesses should remember what the ArrayProfile said rather than relying entirely on the predictions of their children.
Comment 1 Filip Pizlo 2012-08-20 00:29:38 PDT 
Created attachment 159347 [details]
work in progress
Comment 2 Filip Pizlo 2012-08-21 16:32:22 PDT 
Created attachment 159797 [details]
work in progress
Comment 3 Filip Pizlo 2012-08-21 19:38:15 PDT 
Created attachment 159845 [details]
work in progress
Comment 4 Filip Pizlo 2012-08-21 19:46:21 PDT 
Created attachment 159846 [details]
almost there
Comment 5 Filip Pizlo 2012-08-21 20:23:13 PDT 
Created attachment 159851 [details]
getting there
Comment 6 Filip Pizlo 2012-08-22 00:51:26 PDT 
Created attachment 159875 [details]
it runs simple things

Still more testing to go.  And there's also 32-bit to worry about.
Comment 7 Filip Pizlo 2012-08-22 14:22:29 PDT 
Created attachment 160010 [details]
more
Comment 8 Filip Pizlo 2012-08-22 15:57:29 PDT 
Created attachment 160026 [details]
seems to work

Seems at least perf-neutral and it runs non-trivial workloads, but I'm still testing it more.
Comment 9 Filip Pizlo 2012-08-22 17:23:30 PDT 
Created attachment 160041 [details]
the patch
Comment 10 Gavin Barraclough 2012-08-22 18:14:20 PDT 
Comment on attachment 160041 [details]
the patch

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

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:151
> +                    for (unsigned n = 2; n--;)

why? - please to be commenting.

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:328
> +        case PutByVal: {

case PutByValAlias:

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:344
>                  if (m_graph[child3].shouldSpeculateInteger())

if (!condition) fixDoubleEdge
Comment 11 Filip Pizlo 2012-08-22 20:38:43 PDT 
Landed in http://trac.webkit.org/changeset/126387