| Summary: | fourthTier: Add CFG simplification for Switch | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||
| Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | barraclough, benjamin, buildbot, cmarcelo, commit-queue, eflews.bot, ggaren, gyuyoung.kim, mark.lam, mhahnenberg, msaboff, oliver, philn, rego+ews, rniwa, sam, webkit-ews, xan.lopez | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Filip Pizlo
2013-06-15 17:11:06 PDT
Created attachment 204777 [details]
the patch
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.
Comment on attachment 204777 [details] the patch Attachment 204777 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/922198 Comment on attachment 204777 [details] the patch Attachment 204777 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/890288 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 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 Created attachment 204778 [details]
the patch
Fix style
Comment on attachment 204778 [details] the patch Attachment 204778 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/846441 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 Comment on attachment 204778 [details] the patch Attachment 204778 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/890289 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 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 Comment on attachment 204778 [details] the patch Attachment 204778 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/906209 Comment on attachment 204778 [details] the patch Attachment 204778 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/906212 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. (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. Created attachment 204780 [details]
the ptach
And now with tests.
Comment on attachment 204780 [details] the ptach Attachment 204780 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/924189 Comment on attachment 204780 [details] the ptach Attachment 204780 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/867478 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 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 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 Comment on attachment 204780 [details] the ptach Attachment 204780 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/906242 Comment on attachment 204780 [details] the ptach Attachment 204780 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/873287 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? (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. Landed in http://trac.webkit.org/changeset/151644 |