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

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/01ab9094/attachment.html>

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/83306434/attachment.html>

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>