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

On Fri, Apr 22, 2016 at 10:39 AM, Philip Reames <listmail at
philipreames.com>
wrote:
>
>
> On 04/22/2016 09:20 AM, Sanjay Patel via llvm-dev 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.
>
> I don't follow this at all.  Given expects are eagerly lowered to
> metadata, where's the interaction?  In particular, the expect lowering
> overrules any metadata on the associated branch/switch.  This is exactly
> what you'd expect for a user annotation interacting with PGO data.
>
I agree that a user annotation should override PGO data. This is why I
initially proposed that builtin_expect() would extend on
'[un]predictable'
metadata rather than 'prof' metadata in D19299.

But I think that's a separate discussion now; we use 'prof' metadata
for
this purpose today, and I'm not trying to change that in these patches. If
the user-specified representation of builtin_expect() is overwritten by PGO
data, I think that's a bug independent of the current proposal/patches.


>
>
> 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.
>
> Er, what cost?  Given this is a single linear pass over the IR - and could
> actually be made essentially free by checking to see if the module has any
> uses of expect - I'm suspicious of this compile time argument.  Have
you
> actually seen this in profiles?
>
No - I expect the actual overhead is in the noise. The real objection to
the LowerExpectIntrinsic pass is that it is completely unnecessary. This
was raised in the original review. We shouldn't have two mechanisms to
represent exactly the same thing. This is also why my initial
implementation for builtin_unpredictable() was rejected:
http://reviews.llvm.org/D12341

I assumed there was some good reason for LowerExpectIntrinsic to exist, so
I copied that design. As noted in that review, my assumption was wrong. And
it was suggested then that we should remove LowerExpectIntrinsic but nobody
had gotten around to it. With these patches, I'd like to finally fix this.




>
> 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.
>
> This seems like a reasonable proposal.  The expect intrinsic does give us
> a mechanism to express value profiling predictions, but we don't appear
to
> actually use that today.  My bias would be to leave it in place, but
I'm
> not going to object strongly if the consensus goes the other way.
>
>
> 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 listllvm-dev at
lists.llvm.orghttp://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/b65f7ef0/attachment.html>

> On Apr 22, 2016, at 10:27 AM, Sanjay Patel via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> 
> 
> On Fri, Apr 22, 2016 at 10:39 AM, Philip Reames <listmail at
philipreames.com <mailto:listmail at philipreames.com>> wrote:
> 
> 
> On 04/22/2016 09:20 AM, Sanjay Patel via llvm-dev wrote:
>> I've proposed removing the llvm.expect intrinsic:
>> http://reviews.llvm.org/D19300 <http://reviews.llvm.org/D19300>
>> 
>> The motivation for this change is in:
>> http://reviews.llvm.org/D19299 <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.
> I don't follow this at all.  Given expects are eagerly lowered to
metadata, where's the interaction?  In particular, the expect lowering
overrules any metadata on the associated branch/switch.  This is exactly what
you'd expect for a user annotation interacting with PGO data.
> 
> I agree that a user annotation should override PGO data.
PGO is also a user input: the user is basically saying "I want the code to
be optimized for *this* use case".
So interestingly I would have thought the opposite: PGO overrides the source
code annotation.
Here are a couple of reasons why:
 - libraries can be used by different client and what is common in one case
might not for another.
 - code evolves, and user can fail to revisit assumption about the common case
 - the user can be wrong, PGO should not (?).

-- 
Mehdi



> This is why I initially proposed that builtin_expect() would extend on
'[un]predictable' metadata rather than 'prof' metadata in
D19299.
> 
> But I think that's a separate discussion now; we use 'prof'
metadata for this purpose today, and I'm not trying to change that in these
patches. If the user-specified representation of builtin_expect() is overwritten
by PGO data, I think that's a bug independent of the current
proposal/patches.
> 
>  
> 
>> 
>> 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.
> Er, what cost?  Given this is a single linear pass over the IR - and could
actually be made essentially free by checking to see if the module has any uses
of expect - I'm suspicious of this compile time argument.  Have you actually
seen this in profiles?
> 
> No - I expect the actual overhead is in the noise. The real objection to
the LowerExpectIntrinsic pass is that it is completely unnecessary. This was
raised in the original review. We shouldn't have two mechanisms to represent
exactly the same thing. This is also why my initial implementation for
builtin_unpredictable() was rejected:
> http://reviews.llvm.org/D12341 <http://reviews.llvm.org/D12341>
> 
> I assumed there was some good reason for LowerExpectIntrinsic to exist, so
I copied that design. As noted in that review, my assumption was wrong. And it
was suggested then that we should remove LowerExpectIntrinsic but nobody had
gotten around to it. With these patches, I'd like to finally fix this.
> 
> 
> 
>  
>> 
>> 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.
> This seems like a reasonable proposal.  The expect intrinsic does give us a
mechanism to express value profiling predictions, but we don't appear to
actually use that today.  My bias would be to leave it in place, but I'm not
going to object strongly if the consensus goes the other way.
>> 
>> 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
<https://marc.info/?l=llvm-commits&m=130997676129580&w=4>
>> 
>> 
>> 
>> 
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/001eeaa5/attachment-0001.html>

> On Apr 22, 2016, at 10:39 AM, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 
>> 
>> On Apr 22, 2016, at 10:27 AM, Sanjay Patel via llvm-dev <llvm-dev at
lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>> 
>> 
>> 
>> On Fri, Apr 22, 2016 at 10:39 AM, Philip Reames <listmail at
philipreames.com <mailto:listmail at philipreames.com>> wrote:
>> 
>> 
>> On 04/22/2016 09:20 AM, Sanjay Patel via llvm-dev wrote:
>>> I've proposed removing the llvm.expect intrinsic:
>>> http://reviews.llvm.org/D19300
<http://reviews.llvm.org/D19300>
>>> 
>>> The motivation for this change is in:
>>> http://reviews.llvm.org/D19299
<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.
>> I don't follow this at all.  Given expects are eagerly lowered to
metadata, where's the interaction?  In particular, the expect lowering
overrules any metadata on the associated branch/switch.  This is exactly what
you'd expect for a user annotation interacting with PGO data.
>> 
>> I agree that a user annotation should override PGO data.
> 
> PGO is also a user input: the user is basically saying "I want the
code to be optimized for *this* use case".
> So interestingly I would have thought the opposite: PGO overrides the
source code annotation.
> Here are a couple of reasons why:
>  - libraries can be used by different client and what is common in one case
might not for another.
>  - code evolves, and user can fail to revisit assumption about the common
case
>  - the user can be wrong, PGO should not (?).
For this last point, whatever information prevails in the end, it may be
valuable to report to the user some optimization hints about the mismatch
between the PGO measurement and the annotation.
> 
> -- 
> Mehdi
> 
> 
> 
> 
>> This is why I initially proposed that builtin_expect() would extend on
'[un]predictable' metadata rather than 'prof' metadata in
D19299.
>> 
>> But I think that's a separate discussion now; we use 'prof'
metadata for this purpose today, and I'm not trying to change that in these
patches. If the user-specified representation of builtin_expect() is overwritten
by PGO data, I think that's a bug independent of the current
proposal/patches.
>> 
>>  
>> 
>>> 
>>> 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.
>> Er, what cost?  Given this is a single linear pass over the IR - and
could actually be made essentially free by checking to see if the module has any
uses of expect - I'm suspicious of this compile time argument.  Have you
actually seen this in profiles?
>> 
>> No - I expect the actual overhead is in the noise. The real objection
to the LowerExpectIntrinsic pass is that it is completely unnecessary. This was
raised in the original review. We shouldn't have two mechanisms to represent
exactly the same thing. This is also why my initial implementation for
builtin_unpredictable() was rejected:
>> http://reviews.llvm.org/D12341 <http://reviews.llvm.org/D12341>
>> 
>> I assumed there was some good reason for LowerExpectIntrinsic to exist,
so I copied that design. As noted in that review, my assumption was wrong. And
it was suggested then that we should remove LowerExpectIntrinsic but nobody had
gotten around to it. With these patches, I'd like to finally fix this.
>> 
>> 
>> 
>>  
>>> 
>>> 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.
>> This seems like a reasonable proposal.  The expect intrinsic does give
us a mechanism to express value profiling predictions, but we don't appear
to actually use that today.  My bias would be to leave it in place, but I'm
not going to object strongly if the consensus goes the other way.
>>> 
>>> 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
<https://marc.info/?l=llvm-commits&m=130997676129580&w=4>
>>> 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> LLVM Developers mailing list
>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at
lists.llvm.org>
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev>
>> 
>> 
>> _______________________________________________
>> LLVM Developers mailing list
>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/b83b0f54/attachment.html>

yes, PGO data should overrule, and it is a common practice. The annotation
can be stale, and the fixed branch prob implied by the annotation does not
help build up the profile counts/frequency that match the real profile.

David

On Fri, Apr 22, 2016 at 10:39 AM, Mehdi Amini via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> On Apr 22, 2016, at 10:27 AM, Sanjay Patel via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>
>
> On Fri, Apr 22, 2016 at 10:39 AM, Philip Reames <listmail at
philipreames.com
> > wrote:
>
>>
>>
>> On 04/22/2016 09:20 AM, Sanjay Patel via llvm-dev 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.
>>
>> I don't follow this at all.  Given expects are eagerly lowered to
>> metadata, where's the interaction?  In particular, the expect
lowering
>> overrules any metadata on the associated branch/switch.  This is
exactly
>> what you'd expect for a user annotation interacting with PGO data.
>>
>
> I agree that a user annotation should override PGO data.
>
>
> PGO is also a user input: the user is basically saying "I want the
code to
> be optimized for *this* use case".
> So interestingly I would have thought the opposite: PGO overrides the
> source code annotation.
> Here are a couple of reasons why:
>  - libraries can be used by different client and what is common in one
> case might not for another.
>  - code evolves, and user can fail to revisit assumption about the common
> case
>  - the user can be wrong, PGO should not (?).
>
> --
> Mehdi
>
>
>
>
> This is why I initially proposed that builtin_expect() would extend on
> '[un]predictable' metadata rather than 'prof' metadata in
D19299.
>
> But I think that's a separate discussion now; we use 'prof'
metadata for
> this purpose today, and I'm not trying to change that in these patches.
If
> the user-specified representation of builtin_expect() is overwritten by PGO
> data, I think that's a bug independent of the current proposal/patches.
>
>
>
>>
>>
>> 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.
>>
>> Er, what cost?  Given this is a single linear pass over the IR - and
>> could actually be made essentially free by checking to see if the
module
>> has any uses of expect - I'm suspicious of this compile time
argument.
>> Have you actually seen this in profiles?
>>
>
> No - I expect the actual overhead is in the noise. The real objection to
> the LowerExpectIntrinsic pass is that it is completely unnecessary. This
> was raised in the original review. We shouldn't have two mechanisms to
> represent exactly the same thing. This is also why my initial
> implementation for builtin_unpredictable() was rejected:
> http://reviews.llvm.org/D12341
>
> I assumed there was some good reason for LowerExpectIntrinsic to exist, so
> I copied that design. As noted in that review, my assumption was wrong. And
> it was suggested then that we should remove LowerExpectIntrinsic but nobody
> had gotten around to it. With these patches, I'd like to finally fix
this.
>
>
>
>
>
>>
>> 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.
>>
>> This seems like a reasonable proposal.  The expect intrinsic does give
us
>> a mechanism to express value profiling predictions, but we don't
appear to
>> actually use that today.  My bias would be to leave it in place, but
I'm
>> not going to object strongly if the consensus goes the other way.
>>
>>
>> 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 listllvm-dev at
lists.llvm.orghttp://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>>
>>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
>
> _______________________________________________
> 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/6b4b3fc2/attachment.html>