[llvm-dev] [RFC] remove the llvm.expect intrinsic

[Sanjay Patel via llvm-dev]

I, of course, thought the ~100 lines added by D19299 was a reasonable trade
for the ~800 lines removed in D19300.

David Li (and anyone else following along), do you still like those patches
after hearing this objection or should I abandon?


On Fri, Apr 22, 2016 at 11:55 AM, Reid Kleckner <rnk at google.com> wrote:

> Sorry, I didn't realize that was the clang side.
>
> 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.
>
> I think a single early pass to lower this intrinsic is a low cost to pay
> for that simplicity.
>
> On Fri, Apr 22, 2016 at 10:39 AM, Sanjay Patel <spatel at rotateright.com>
> wrote:
>
>> Hi Reid -
>>
>> The intent of D19299 is to remove all Clang refs to llvm.expect. Do you
>> see any holes after applying that patch?
>>
>> On Fri, Apr 22, 2016 at 11:36 AM, Reid Kleckner <rnk at google.com> wrote:
>>
>>> 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.
>>>
>>> On Fri, Apr 22, 2016 at 9:20 AM, Sanjay Patel via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> I've proposed removing the llvm.expect intrinsic:
>>>> http://reviews.llvm.org/D19300
>>>>
>>>> The motivation for this change is in:
>>>> http://reviews.llvm.org/D19299
>>>>
>>>> For reference:
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>> 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.
>>>>
>>>> Please let me know if you see any problems with this proposal or the
>>>> patches.
>>>>
>>>> For reference, here's the original post-commit review thread for
>>>> llvm.expect:
>>>> https://marc.info/?l=llvm-commits&m=130997676129580&w=4
>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20160422/c28a56bd/attachment.html>