Summary: | DFG::LICMPhase shouldn't hoist type checks unless it knows that the check will succeed at the loop pre-header | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||||||
Component: | JavaScriptCore | Assignee: | Filip Pizlo <fpizlo> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | barraclough, benjamin, commit-queue, ggaren, keith_miller, mark.lam, mhahnenb, msaboff, oliver, ossy, saam, sam | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | All | ||||||||||||||
OS: | All | ||||||||||||||
Attachments: |
|
Description
Filip Pizlo
2015-05-02 11:35:59 PDT
Created attachment 279138 [details]
it begins!
Created attachment 279152 [details]
more
Created attachment 279298 [details]
almost done
Not landing this yet because it's a massive regression on imaging-gaussian-blur. Basically we need to be somewhat careful about what we consider to be too unsafe to hoist. Perhaps on the first compilation we should just allow everything to be hoisted. I think that if nothing that was hoisted has ever exited then we should allow ourselves to hoist without worrying. If anything that was hoisted has exited then we should be more careful about hoisting. So, I think that we want something in the exit profile that: 1) Makes sure that hoisted exits don't contribute to the normal exit counting. 2) Makes sure that we know if hoisted exits had ever happened. Created attachment 279348 [details]
the patch
Created attachment 279394 [details]
the patch
Attachment 279394 [details] did not pass style-queue:
ERROR: Source/WTF/wtf/BackwardsGraph.h:174: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGGraph.cpp:35: Alphabetical sorting problem. [build/include_order] [4]
ERROR: Source/JavaScriptCore/dfg/DFGMayExit.cpp:118: Place brace on its own line for function definitions. [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:106: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/dfg/DFGControlEquivalenceAnalysis.h:31: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 5 in 41 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #8) > Attachment 279394 [details] did not pass style-queue: > > > ERROR: Source/WTF/wtf/BackwardsGraph.h:174: Place brace on its own line for > function definitions. [whitespace/braces] [4] > ERROR: Source/JavaScriptCore/dfg/DFGGraph.cpp:35: Alphabetical sorting > problem. [build/include_order] [4] > ERROR: Source/JavaScriptCore/dfg/DFGMayExit.cpp:118: Place brace on its own > line for function definitions. [whitespace/braces] [4] > ERROR: Source/JavaScriptCore/ftl/FTLOSRExit.cpp:106: Wrong number of spaces > before statement. (expected: 8) [whitespace/indent] [4] > ERROR: Source/JavaScriptCore/dfg/DFGControlEquivalenceAnalysis.h:31: > Alphabetical sorting problem. [build/include_order] [4] > Total errors found: 5 in 41 files > > > If any of these errors are false positives, please file a bug against > check-webkit-style. I fixed the style errors that were real style errors. Comment on attachment 279394 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=279394&action=review r=me > Source/JavaScriptCore/ChangeLog:9 > + This adds a control flow equivalence analysis (called ControlEquivalenceAnalysis) based on > + dominator analysis over the backwards CFG. LICM now uses it to become more conservative Worth having a sentence or two here explaining control flow equivalence for posterity. > Source/JavaScriptCore/dfg/DFGBackwardsDominators.h:43 > + : WTF::Dominators<BackwardsCFG>(graph.ensureBackwardsCFG()) Nice! Pretty sweet that we can just do this with other graphs too. > Source/WTF/wtf/BackwardsGraph.h:204 > + template<typename T> > + Map<T> newMap() { return Map<T>(m_graph); } Nit: Some of these can probably be private. (In reply to comment #10) > Comment on attachment 279394 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279394&action=review > > r=me > > > Source/JavaScriptCore/ChangeLog:9 > > + This adds a control flow equivalence analysis (called ControlEquivalenceAnalysis) based on > > + dominator analysis over the backwards CFG. LICM now uses it to become more conservative > > Worth having a sentence or two here explaining control flow equivalence for > posterity. Will do. > > > Source/JavaScriptCore/dfg/DFGBackwardsDominators.h:43 > > + : WTF::Dominators<BackwardsCFG>(graph.ensureBackwardsCFG()) > > Nice! > Pretty sweet that we can just do this with other graphs too. > > > Source/WTF/wtf/BackwardsGraph.h:204 > > + template<typename T> > > + Map<T> newMap() { return Map<T>(m_graph); } > > Nit: Some of these can probably be private. Actually newMap() is part of the API that a Graph must provide to Dominators, so it has to be public. Landed in http://trac.webkit.org/changeset/201182 (In reply to comment #12) > Landed in http://trac.webkit.org/changeset/201182 It broke the build everywhere. (In reply to comment #13) > (In reply to comment #12) > > Landed in http://trac.webkit.org/changeset/201182 > > It broke the build everywhere. Fix coming. (In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #12) > > > Landed in http://trac.webkit.org/changeset/201182 > > > > It broke the build everywhere. > > Fix coming. This was caused by an idiotic last-minute change where I hoisted a string constant into a constant variable. I forgot to rebuild and retest. I landed a speculative fix as soon as I realized my mistake: http://trac.webkit.org/changeset/201184 Then I landed a proper fix once my build caught up and actually compiled the code: http://trac.webkit.org/changeset/201185 If this does not fix it then I'm ready to roll it out. |