<div dir="ltr">Your patch does make the handling of __builtin_unpredictable and __builtin_expect 'unified' in some way.<div><br></div><div>However given the point brought up by Reid, and the rationale by Richard, as well as consideration of the easiness for enhancement in the future, I think keep the current mechanism is a better approach. </div><div><br></div><div>The test cases from Richard are broken with current lowering, but it is just bugs to be fixed.</div><div><br></div><div>David</div></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:0 0 0 .8ex;border-left:1px #ccc 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="HOEnZb"><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:0 0 0 .8ex;border-left:1px #ccc 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:0 0 0 .8ex;border-left:1px #ccc 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:0 0 0 .8ex;border-left:1px #ccc 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:0 0 0 .8ex;border-left:1px #ccc 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>