<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Aug 28, 2020 at 10:53 PM Nikita Popov <<a href="mailto:nikita.ppv@gmail.com">nikita.ppv@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 27, 2020 at 11:01 PM Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, Aug 27, 2020 at 10:43 PM Roman Lebedev <<a href="mailto:lebedev.ri@gmail.com" target="_blank">lebedev.ri@gmail.com</a>> wrote:<br>
><br>
> On Thu, Aug 27, 2020 at 9:55 PM Nikita Popov <<a href="mailto:nikita.ppv@gmail.com" target="_blank">nikita.ppv@gmail.com</a>> wrote:<br>
> ><br>
> > On Thu, Aug 27, 2020 at 5:47 PM Roman Lebedev via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> >><br>
> >><br>
> >> Author: Roman Lebedev<br>
> >> Date: 2020-08-27T18:47:04+03:00<br>
> >> New Revision: 6102310d814ad73eab60a88b21dd70874f7a056f<br>
> >><br>
> >> URL: <a href="https://github.com/llvm/llvm-project/commit/6102310d814ad73eab60a88b21dd70874f7a056f" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/6102310d814ad73eab60a88b21dd70874f7a056f</a><br>
> >> DIFF: <a href="https://github.com/llvm/llvm-project/commit/6102310d814ad73eab60a88b21dd70874f7a056f.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/6102310d814ad73eab60a88b21dd70874f7a056f.diff</a><br>
> >><br>
> >> LOG: [InstSimplify][EarlyCSE] Try to CSE PHI nodes in the same basic block<br>
> >><br>
> >> Apparently, we don't do this, neither in EarlyCSE, nor in InstSimplify,<br>
> >> nor in (old) GVN, but do in NewGVN and SimplifyCFG of all places..<br>
> >><br>
> >> While i could teach EarlyCSE how to hash PHI nodes,<br>
> >> we can't really do much (anything?) even if we find two identical<br>
> >> PHI nodes in different basic blocks, same-BB case is the interesting one,<br>
> >> and if we teach InstSimplify about it (which is what i wanted originally,<br>
> >> <a href="https://reviews.llvm.org/D86530" rel="noreferrer" target="_blank">https://reviews.llvm.org/D86530</a>), we get EarlyCSE support for free.<br>
> >><br>
> >> So i would think this is pretty uncontroversial.<br>
> >><br>
> >> On vanilla llvm test-suite + RawSpeed, this has the following effects:<br>
> >> ```<br>
> >> | statistic name | baseline | proposed | Δ | % | \|%\| |<br>
> >> |----------------------------------------------------|-----------|-----------|-------:|---------:|---------:|<br>
> >> | instsimplify.NumPHICSE | 0 | 23779 | 23779 | 0.00% | 0.00% |<br>
> >> | asm-printer.EmittedInsts | 7942328 | 7942392 | 64 | 0.00% | 0.00% |<br>
> >> | assembler.ObjectBytes | 273069192 | 273084704 | 15512 | 0.01% | 0.01% |<br>
> >> | correlated-value-propagation.NumPhis | 18412 | 18539 | 127 | 0.69% | 0.69% |<br>
> >> | early-cse.NumCSE | 2183283 | 2183227 | -56 | 0.00% | 0.00% |<br>
> >> | early-cse.NumSimplify | 550105 | 542090 | -8015 | -1.46% | 1.46% |<br>
> >> | instcombine.NumAggregateReconstructionsSimplified | 73 | 4506 | 4433 | 6072.60% | 6072.60% |<br>
> >> | instcombine.NumCombined | 3640264 | 3664769 | 24505 | 0.67% | 0.67% |<br>
> >> | instcombine.NumDeadInst | 1778193 | 1783183 | 4990 | 0.28% | 0.28% |<br>
> >> | instcount.NumCallInst | 1758401 | 1758799 | 398 | 0.02% | 0.02% |<br>
> >> | instcount.NumInvokeInst | 59478 | 59502 | 24 | 0.04% | 0.04% |<br>
> >> | instcount.NumPHIInst | 330557 | 330533 | -24 | -0.01% | 0.01% |<br>
> >> | instcount.TotalInsts | 8831952 | 8832286 | 334 | 0.00% | 0.00% |<br>
> >> | simplifycfg.NumInvokes | 4300 | 4410 | 110 | 2.56% | 2.56% |<br>
> >> | simplifycfg.NumSimpl | 1019808 | 999607 | -20201 | -1.98% | 1.98% |<br>
> >> ```<br>
> >> I.e. it fires ~24k times, causes +110 (+2.56%) more `invoke` -> `call`<br>
> >> transforms, and counter-intuitively results in *more* instructions total.<br>
> >><br>
> >> That being said, the PHI count doesn't decrease that much,<br>
> >> and looking at some examples, it seems at least some of them<br>
> >> were previously getting PHI CSE'd in SimplifyCFG of all places..<br>
> >><br>
> >> I'm adjusting `Instruction::isIdenticalToWhenDefined()` at the same time.<br>
> >> As a comment in `InstCombinerImpl::visitPHINode()` already stated,<br>
> >> there are no guarantees on the ordering of the operands of a PHI node,<br>
> >> so if we just naively compare them, we may false-negatively say that<br>
> >> the nodes are not equal when the only difference is operand order,<br>
> >> which is especially important since the fold is in InstSimplify,<br>
> >> so we can't rely on InstCombine sorting them beforehand.<br>
> >><br>
> >> Fixing this for the general case is costly (geomean +0.02%),<br>
> >> and does not appear to catch anything in test-suite, but for<br>
> >> the same-BB case, it's trivial, so let's fix at least that.<br>
> >><br>
> >> As per <a href="http://llvm-compile-time-tracker.com/compare.php?from=04879086b44348cad600a0a1ccbe1f7776cc3cf9&to=82bdedb888b945df1e9f130dd3ac4dd3c96e2925&stat=instructions" rel="noreferrer" target="_blank">http://llvm-compile-time-tracker.com/compare.php?from=04879086b44348cad600a0a1ccbe1f7776cc3cf9&to=82bdedb888b945df1e9f130dd3ac4dd3c96e2925&stat=instructions</a><br>
> >> this appears to cause geomean +0.03% compile time increase (regression),<br>
> >> but geomean -0.01%..-0.04% code size decrease (improvement).<br>
><br>
> > This transform seems to be breaking some new ground for InstSimplify, in that it can<br>
> > now simplify to instructions that are not def-use "reachable" from the root instruction.<br>
> True.<br>
><br>
> > I'm a bit uncomfortable with this as a general direction.<br>
> Please can you be more specific?<br>
><br>
> > Would we lose out much transformation power if we did this in InstCombine only instead?<br>
> I'm not really sure, let me see how it affects -stats i guess.<br>
Effect of moving it to instcombine: <a href="https://reviews.llvm.org/P8233" rel="noreferrer" target="_blank">https://reviews.llvm.org/P8233</a><br>
(again, vanilla llvm test-suite + RawSpeed)<br>
So having it as a InstSimplify utility catches ~7% more PHI's.<br>
Honestly i expected this number to be *much* lower.<br></blockquote><div><br></div><div>Thanks for testing! I think it should be expected that doing something in InstSimplify will trigger more, simplify by dint of running earlier and more often (i.e., before other folds happen). It doesn't look like this change affects the motivating statistics (simplifycfg.NumInvokes and instcombine.NumAggregateReconstructionsSimplified remain the same).<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I think i'd like to better understand what breakage keeping it in<br>
InstSimplify causes.<br></blockquote><div><br></div><div>My concern here is more about the pass design than any actual breakage. Performing CSE is outside the scope of InstSimplify responsibility. We have other passes, such as EarlyCSE and GVN that are responsible for CSE.</div><div><br></div><div>Now, InstCombine is of course also not responsible for performing CSE. However, InstCombine is already a rather pragmatic kitchen sink of random transforms and already contains quite a few CSE related optimizations, such as basic redundant load elimination. Adding Phi CSE support to it doesn't change the types of optimizations InstCombine performs in a fundamental way, just adds one more special case.</div></div><div class="gmail_quote"><br></div><div class="gmail_quote">Regards,<br></div><div class="gmail_quote">Nikita </div></div></blockquote><div><br></div><div>As for actual breakage: I have a strong suspicion that this change has introduced compiler non-determinism. Looking at <a href="https://llvm-compile-time-tracker.com/index.php?config=ReleaseThinLTO&stat=size-text">https://llvm-compile-time-tracker.com/index.php?config=ReleaseThinLTO&stat=size-text</a> text size fluctuations have started occurring directly after this commit.</div><div><br></div><div>Regards,<br></div><div>Nikita<br> </div></div></div>