<html><head><meta http-equiv="Content-Type" content="text/html; charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On Mar 28, 2018, at 4:38 PM, Daniel Berlin <<a href="mailto:dberlin@dberlin.org" class="">dberlin@dberlin.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">I'm ... confused.<div class=""><br class=""></div><div class="gmail_extra"><br class=""><div class="gmail_quote">On Wed, Mar 28, 2018 at 4:33 PM, Michael Zolotukhin via Phabricator <span dir="ltr" class=""><<a href="mailto:reviews@reviews.llvm.org" target="_blank" class="">reviews@reviews.llvm.org</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mzolotukhin added a comment.<br class="">
<span class=""><br class="">
> I don't mind this approach, but as discussed offline we should consider also moving LCSSA to make sure the API makes sense.<br class="">
> In general, what's your plan for this? You want it to replace the SSA updater for all the instances in llvm? If so, we should carefully plan the transition costs.<br class="">
<br class="">
</span>I looked into LCSSA, and indeed it seems that it also can be improved. I tried direct replacement of the old SSAUpdater with the new one, but that didn't give any benefits. </blockquote><div class=""><br class=""></div><div class="">Before you said "<span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline" class="">(although the new implementation still was </span><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline" class="">significantly faster than the existing one)”</span></div></div></div></div></div></blockquote><div>Let’s clarify this. “Although the new implementation still was significantly faster” was about another approach based on D28934.</div><div><br class=""></div><div>This patch (let’s call it SSAUpdaterBulk) is about another approach, which is closer to what we have in the trunk (let’s call it SSAUpdater) except that we 1) don’t compute DT but instead use DT provided by the user, 2) can do bulk updates that share the same DT and IDF when possible. If the use case is that we have a lot of uses to rewrite and they share the same definitions, SSAUpdaterBulk should be faster than SSAUpdater, as it won’t compute DT at all and will compute IDF only once for all these defs and uses. That’s what happens in JumpThreading and that’s why we see the gains there. If, however, we have an opposite case, where we rewrite one use at a time, we recompute IDF every time we do the rewrite, which is not so much faster and sometimes probably even slower than what we have in SSAUpdater. That’s probably what happened when I tried a direct replacement of SSAUpdater with SSAUpdaterBulk in LCSSA. What I was saying was that it should be possible to rewrite LCSSA in a way allowing to get benefits of SSAUpdaterBulk.</div><div><br class=""></div><div>Hopefully it helps a it and sorry if it’s been confusing!</div><div><br class=""></div><div>Thanks,</div>Michael<br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class=""><span style="color:rgb(34,34,34);font-family:arial,sans-serif;font-size:12.8px;font-style:normal;font-variant-ligatures:normal;font-variant-caps:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;background-color:rgb(255,255,255);text-decoration-style:initial;text-decoration-color:initial;float:none;display:inline" class=""><br class=""></span></div></div></div></div>
</div></blockquote></div><br class=""></body></html>