Patch forthcoming.
Created attachment 316175 [details] it's a start
Created attachment 316194 [details] more
Created attachment 316195 [details] more The LICM itself is written, now I need to write the pre-header creator.
Created attachment 316196 [details] wrote the pre-header creation
Actually, as awesome as this is, I don't think this is the way we want to handle optimizing caging. If a butterfly is used both caged and uncaged, we want to do the caging once and drop the uncaged value just to relieve register pressure.
Created attachment 316223 [details] more
Created attachment 316229 [details] better version I realized that InnerMostLoopIndices should be managed by a typename Graph::template Map.
Created attachment 316230 [details] added comprehensive tests and made them pass
Created attachment 316231 [details] added changelog
Created attachment 316232 [details] rebased
Created attachment 316241 [details] the patch
Comment on attachment 316241 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=316241&action=review r=me with some nits. > Source/JavaScriptCore/ChangeLog:8 > + Added a LICM phase to B3. This phase is called hoistLoopInvariantValues, to conform to the B3 naming I dunno HLIV doesn't roll off the tongue the same way LICM does... > Source/JavaScriptCore/b3/B3BasicBlock.cpp:60 > - m_values[m_values.size() - 1] = value; > + m_values[m_values.size() - 2] = value; legit... > Source/JavaScriptCore/b3/B3CFG.h:62 > + revert please. > Source/JavaScriptCore/b3/B3EnsureLoopPreHeaders.cpp:71 > + loop.header()->addPredecessor(preHeader); Nit: It would probably be a little clearer if you moved this line to where you set the successor of the preHeader.
Comment on attachment 316241 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=316241&action=review > Source/WTF/wtf/NaturalLoops.h:279 > + return 0; Nit: nullptr > Source/WTF/wtf/NaturalLoops.h:293 > + return 0; ditto > Source/WTF/wtf/NaturalLoops.h:300 > + return 0; ditto
Comment on attachment 316241 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=316241&action=review LGTM too > Source/JavaScriptCore/ChangeLog:20 > + This isn't super important. It's perf-neutral on JS benchmarks. FTL already does LICM on DFG SSA IR, and > + probably all current WebAssembly content has had LICM done to it. That being said, this is a cheap phase > + so it doesn't hurt to have it. It'd be interesting to compile some Wasm using -O0 and see if this is a speedup. I'd bet it is.
(In reply to Saam Barati from comment #14) > Comment on attachment 316241 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316241&action=review > > LGTM too > > > Source/JavaScriptCore/ChangeLog:20 > > + This isn't super important. It's perf-neutral on JS benchmarks. FTL already does LICM on DFG SSA IR, and > > + probably all current WebAssembly content has had LICM done to it. That being said, this is a cheap phase > > + so it doesn't hurt to have it. > > It'd be interesting to compile some Wasm using -O0 and see if this is a > speedup. I'd bet it is. I'm not sure it would be since we don't have abstract heaps yet in wasm. That was on my todo but I never got around to it. But we should totally do this...
(In reply to Keith Miller from comment #15) > (In reply to Saam Barati from comment #14) > > Comment on attachment 316241 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=316241&action=review > > > > LGTM too > > > > > Source/JavaScriptCore/ChangeLog:20 > > > + This isn't super important. It's perf-neutral on JS benchmarks. FTL already does LICM on DFG SSA IR, and > > > + probably all current WebAssembly content has had LICM done to it. That being said, this is a cheap phase > > > + so it doesn't hurt to have it. > > > > It'd be interesting to compile some Wasm using -O0 and see if this is a > > speedup. I'd bet it is. > > I'm not sure it would be since we don't have abstract heaps yet in wasm. > That was on my todo but I never got around to it. But we should totally do > this... It'll be a speed-up if we hoist division or multiplication. Also, for float code, if you can hoist basically anything then you get faster.
Comment on attachment 316241 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=316241&action=review >> Source/JavaScriptCore/b3/B3EnsureLoopPreHeaders.cpp:71 >> + loop.header()->addPredecessor(preHeader); > > Nit: It would probably be a little clearer if you moved this line to where you set the successor of the preHeader. Good point. Will do.
(In reply to Saam Barati from comment #13) > Comment on attachment 316241 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316241&action=review > > > Source/WTF/wtf/NaturalLoops.h:279 > > + return 0; > > Nit: nullptr > > > Source/WTF/wtf/NaturalLoops.h:293 > > + return 0; > > ditto > > > Source/WTF/wtf/NaturalLoops.h:300 > > + return 0; > > ditto Fixed.
(In reply to Keith Miller from comment #12) > Comment on attachment 316241 [details] > the patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=316241&action=review > > r=me with some nits. > > > Source/JavaScriptCore/ChangeLog:8 > > + Added a LICM phase to B3. This phase is called hoistLoopInvariantValues, to conform to the B3 naming > > I dunno HLIV doesn't roll off the tongue the same way LICM does... > > > Source/JavaScriptCore/b3/B3BasicBlock.cpp:60 > > - m_values[m_values.size() - 1] = value; > > + m_values[m_values.size() - 2] = value; > > legit... > > > Source/JavaScriptCore/b3/B3CFG.h:62 > > + > > revert please. Fixed. > > > Source/JavaScriptCore/b3/B3EnsureLoopPreHeaders.cpp:71 > > + loop.header()->addPredecessor(preHeader); > > Nit: It would probably be a little clearer if you moved this line to where > you set the successor of the preHeader. Fixed.
(In reply to Filip Pizlo from comment #19) > (In reply to Keith Miller from comment #12) > > Comment on attachment 316241 [details] > > the patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=316241&action=review > > > > r=me with some nits. > > > > > Source/JavaScriptCore/ChangeLog:8 > > > + Added a LICM phase to B3. This phase is called hoistLoopInvariantValues, to conform to the B3 naming > > > > I dunno HLIV doesn't roll off the tongue the same way LICM does... > > > > > Source/JavaScriptCore/b3/B3BasicBlock.cpp:60 > > > - m_values[m_values.size() - 1] = value; > > > + m_values[m_values.size() - 2] = value; > > > > legit... > > > > > Source/JavaScriptCore/b3/B3CFG.h:62 > > > + > > > > revert please. > > Fixed. > > > > > > Source/JavaScriptCore/b3/B3EnsureLoopPreHeaders.cpp:71 > > > + loop.header()->addPredecessor(preHeader); > > > > Nit: It would probably be a little clearer if you moved this line to where > > you set the successor of the preHeader. > > Fixed. Actually no! My original ordering means that we first remove predecessors and then add. Your reordering, while clearer, would mean adding and then removing. It's a bit more efficient to remove then add, so I'll keep the current ordering.
Landed in https://trac.webkit.org/changeset/219898/webkit