<div dir="ltr"><div class="gmail_extra">As I recall, the reason we chose to emit this via an intrinsic from Clang rather than directly as profile information was to catch cases where the builtin is *not* the immediate operand of a control flow construct, but would be after some simple optimization. For instance:</div><div class="gmail_extra"><br></div><div class="gmail_extra"> bool b = __builtin_expect(n > 5, true);</div><div class="gmail_extra"> if (b) { ... }</div><div class="gmail_extra"> // ...</div><div class="gmail_extra"> if (b) { ... }</div><div class="gmail_extra"><br></div><div class="gmail_extra">or</div><div class="gmail_extra"><br></div><div class="gmail_extra"> if (x && __builtin_expect(y, true)) { ... }</div><div class="gmail_extra"><br></div><div class="gmail_extra">It sounds like subsequent changes to LLVM have already made most of these cases not work, so some of the motivation for the current approach may have evaporated. What optimizations do we still perform before lowering the expect intrinsics?</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 22, 2016 at 1:50 PM, Sanjay Patel via cfe-dev <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div><div>I, of course, thought the ~100 lines added by D19299 was a reasonable trade for the ~800 lines removed in D19300.<br></div><br>David Li (and anyone else following along), do you still like those patches after hearing this objection or should I abandon?<br><br></div></div><div class=""><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 22, 2016 at 11:55 AM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Sorry, I didn't realize that was the clang side.</div><div class="gmail_quote"><br></div><div class="gmail_quote">I think it's kind of ugly that the frontend has to pattern match and lower this intrinsic in three places: if, switch, and general case. The existing intrinsic is nice because it directly represents what the user can write, and lets the optimizer decide how to best represent it. As David Li mentioned, we may want to do value profiling in the future, and if we remove this intrinsic, we will have to come and revisit this code.<br><br>I think a single early pass to lower this intrinsic is a low cost to pay for that simplicity.</div><div><div><div class="gmail_quote"><br></div><div class="gmail_quote">On Fri, Apr 22, 2016 at 10:39 AM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr"><div>Hi Reid -<br><br></div>The intent of D19299 is to remove all Clang refs to llvm.expect. Do you see any holes after applying that patch? <br></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 22, 2016 at 11:36 AM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Clang still appears to use llvm.expect. I think if you can show that it's trivial to update clang with a patch, then yeah, moving back down to one representation for this sounds reasonable.</div><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Fri, Apr 22, 2016 at 9:20 AM, Sanjay Patel via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div><div dir="ltr"><div><div>I've proposed removing the llvm.expect intrinsic:<br><a href="http://reviews.llvm.org/D19300" target="_blank">http://reviews.llvm.org/D19300</a><br><br></div>The motivation for this change is in:<br><a href="http://reviews.llvm.org/D19299" target="_blank">http://reviews.llvm.org/D19299</a><br><br>For reference:<br>1. We created an intrinsic that's only reason for existing is to improve perf, but the
intrinsic can harm optimization by interfering with transforms in other
passes.<br><br>2. To solve that, we created a pass to always
transform the intrinsic into metadata at a very early stage in LLVM. But now
every program is paying a compile-time tax (running the LowerExpectIntrinsic pass) for a feature that is rarely
used.<br><br>A possible front-end replacement transformation for a source-level "builtin_expect()" is in D19299: I think a front-end can always directly create metadata for this kind of programmer hint rather than using an intermediate representation (the intrinsic). Likewise, if there's some out-of-tree IR pass that is creating an llvm.expect, it should be able to create branch weight metadata directly instead.<br><br>Please let me know if you see any problems with this proposal or the patches.<br><br></div><div>For reference, here's the original post-commit review thread for llvm.expect:<br><a href="https://marc.info/?l=llvm-commits&m=130997676129580&w=4" rel="noreferrer" target="_blank">https://marc.info/?l=llvm-commits&m=130997676129580&w=4</a><br><br></div><div><br></div></div>
<br></div></div><span>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
<br></span></blockquote></div><br></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div></div>
</blockquote></div><br></div>
</div></div><br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div></div>