Bug 117677 – fourthTier: Add CFG simplification for Switch

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

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 204777 [details]
the patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp', u'Source/JavaScriptCore/runtime/JSCJSValue.h', u'Source/JavaScriptCore/runtime/JSCJSValueInlines.h']" exit_code: 1
Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:313:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:320:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:327:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 3 in 4 files


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

[EFL EWS Bot]

Comment on attachment 204777 [details]
the patch

Attachment 204777 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/922198

[Early Warning System Bot]

Comment on attachment 204777 [details]
the patch

Attachment 204777 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/890288

[EFL EWS Bot]

Comment on attachment 204777 [details]
the patch

Attachment 204777 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/873251

[Early Warning System Bot]

Comment on attachment 204777 [details]
the patch

Attachment 204777 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/873249

[Filip Pizlo]

Created attachment 204778 [details]
the patch

Fix style

[EFL EWS Bot]

Comment on attachment 204778 [details]
the patch

Attachment 204778 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/846441

[EFL EWS Bot]

Comment on attachment 204778 [details]
the patch

Attachment 204778 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/855477

[Early Warning System Bot]

Comment on attachment 204778 [details]
the patch

Attachment 204778 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/890289

[Early Warning System Bot]

Comment on attachment 204778 [details]
the patch

Attachment 204778 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/855478

[Build Bot]

Comment on attachment 204778 [details]
the patch

Attachment 204778 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/920181

[Build Bot]

Comment on attachment 204778 [details]
the patch

Attachment 204778 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/906209

[Build Bot]

Comment on attachment 204778 [details]
the patch

Attachment 204778 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/906212

[Sam Weinig]

It would be nice for the ChangeLog to have a bit of information in it, like why this is a good idea, and if it speeds anything up.

[Filip Pizlo]

(In reply to comment #15)
> It would be nice for the ChangeLog to have a bit of information in it, like why this is a good idea, and if it speeds anything up.

It's for completeness.  It only speeds up a microbenchmark at this point.  But broadly, we want all control constructs to be known to the CFG simplifier.

I'll add that text to the ChangeLog.

[Filip Pizlo]

Created attachment 204780 [details]
the ptach

And now with tests.

[EFL EWS Bot]

Comment on attachment 204780 [details]
the ptach

Attachment 204780 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/924189

[Early Warning System Bot]

Comment on attachment 204780 [details]
the ptach

Attachment 204780 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/867478

[EFL EWS Bot]

Comment on attachment 204780 [details]
the ptach

Attachment 204780 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/902228

[Early Warning System Bot]

Comment on attachment 204780 [details]
the ptach

Attachment 204780 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/920203

[Build Bot]

Comment on attachment 204780 [details]
the ptach

Attachment 204780 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/920201

[Build Bot]

Comment on attachment 204780 [details]
the ptach

Attachment 204780 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/906242

[Build Bot]

Comment on attachment 204780 [details]
the ptach

Attachment 204780 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/873287

[Mark Hahnenberg]

Comment on attachment 204780 [details]
the ptach

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

r=me

> Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:190
> +                        for (unsigned i = data->cases.size(); found == FalseTriState && i--;) {

What happens if there are multiple case statements with the same constants?

[Filip Pizlo]

(In reply to comment #25)
> (From update of attachment 204780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204780&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp:190
> > +                        for (unsigned i = data->cases.size(); found == FalseTriState && i--;) {
> 
> What happens if there are multiple case statements with the same constants?

Won't happen since the bytecode's jump tables never have duplicates.  I'll add a comment at the declaration of SwitchCase.

[Filip Pizlo]

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