RFC: Clang-Misexpect Proposal

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().  
 
--- TLDR ---
What are we proposing?
* Adding a new tool, clang-mispredict, to the LLVM project.
 
What is its purpose?
* Identifying potentially problematic uses of __builtin_expect().
 
Why is this important?
* __builtin_expect() has a high performance cost when it is used incorrectly.
 
Where will the code live?
* clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
 
Open questions
* What is the correct heuristic for determining a mismatch?
 
Potential shortcomings
* The approach is sensitive to both profiling input and configuration.
 
 
--- Details ---
 
We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
 
Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
 
Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
 
We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
 
The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
 
Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
 
Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
 
Please let us know if you have comments or concerns about this proposal.

Thanks!

--
Paul Kirth

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
Sounds like a useful tool.

Could it be enough for clang to emit warnings in PGO mode when it
detects a mismatch between collected profile data and
__builtin_expect() ?

Michael


Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
<[hidden email]>:

>
> We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>
> --- TLDR ---
> What are we proposing?
> * Adding a new tool, clang-mispredict, to the LLVM project.
>
> What is its purpose?
> * Identifying potentially problematic uses of __builtin_expect().
>
> Why is this important?
> * __builtin_expect() has a high performance cost when it is used incorrectly.
>
> Where will the code live?
> * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>
> Open questions
> * What is the correct heuristic for determining a mismatch?
>
> Potential shortcomings
> * The approach is sensitive to both profiling input and configuration.
>
>
> --- Details ---
>
> We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>
> Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>
> Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>
> We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>
> The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>
> Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>
> Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>
> Please let us know if you have comments or concerns about this proposal.
>
> Thanks!
>
> --
> Paul Kirth
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev


On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
Sounds like a useful tool.


Thanks! I certainly hope it will be.
 
Could it be enough for clang to emit warnings in PGO mode when it
detects a mismatch between collected profile data and
__builtin_expect() ?


You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.

However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline. 

Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile. When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.

That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang. 

Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
 
Michael


Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
<[hidden email]>:
>
> We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>
> --- TLDR ---
> What are we proposing?
> * Adding a new tool, clang-mispredict, to the LLVM project.
>
> What is its purpose?
> * Identifying potentially problematic uses of __builtin_expect().
>
> Why is this important?
> * __builtin_expect() has a high performance cost when it is used incorrectly.
>
> Where will the code live?
> * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>
> Open questions
> * What is the correct heuristic for determining a mismatch?
>
> Potential shortcomings
> * The approach is sensitive to both profiling input and configuration.
>
>
> --- Details ---
>
> We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>
> Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>
> Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>
> We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>
> The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>
> Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>
> Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>
> Please let us know if you have comments or concerns about this proposal.
>
> Thanks!
>
> --
> Paul Kirth
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


--
Paul Kirth

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
<[hidden email]> wrote:

>
>
>
> On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>>
>> Sounds like a useful tool.
>>
>
> Thanks! I certainly hope it will be.
>
>>
>> Could it be enough for clang to emit warnings in PGO mode when it
>> detects a mismatch between collected profile data and
>> __builtin_expect() ?
>>
>
> You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>
> However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>
> Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
Can those be simply reported as normal Remarks?
Those can later be visualized over the codebase via opt-viewer.

> When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>
> That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>
> Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>
>>
>> Michael
>>
>>
>> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> <[hidden email]>:
>> >
>> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >
>> > --- TLDR ---
>> > What are we proposing?
>> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >
>> > What is its purpose?
>> > * Identifying potentially problematic uses of __builtin_expect().
>> >
>> > Why is this important?
>> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >
>> > Where will the code live?
>> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >
>> > Open questions
>> > * What is the correct heuristic for determining a mismatch?
>> >
>> > Potential shortcomings
>> > * The approach is sensitive to both profiling input and configuration.
>> >
>> >
>> > --- Details ---
>> >
>> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >
>> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >
>> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >
>> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >
>> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >
>> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >
>> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >
>> > Please let us know if you have comments or concerns about this proposal.
>> >
>> > Thanks!
>> >
>> > --
>> > Paul Kirth
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
> --
> Paul Kirth
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

Roman
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev


On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
<[hidden email]> wrote:
>
>
>
> On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>>
>> Sounds like a useful tool.
>>
>
> Thanks! I certainly hope it will be.
>
>>
>> Could it be enough for clang to emit warnings in PGO mode when it
>> detects a mismatch between collected profile data and
>> __builtin_expect() ?
>>
>
> You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>
> However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>
> Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
Can those be simply reported as normal Remarks?
Those can later be visualized over the codebase via opt-viewer.


Could you clarify the context here for me? I'm not sure if you mean:

1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).

If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler. 

I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.

 
> When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>
> That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>
> Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>
>>
>> Michael
>>
>>
>> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> <[hidden email]>:
>> >
>> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >
>> > --- TLDR ---
>> > What are we proposing?
>> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >
>> > What is its purpose?
>> > * Identifying potentially problematic uses of __builtin_expect().
>> >
>> > Why is this important?
>> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >
>> > Where will the code live?
>> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >
>> > Open questions
>> > * What is the correct heuristic for determining a mismatch?
>> >
>> > Potential shortcomings
>> > * The approach is sensitive to both profiling input and configuration.
>> >
>> >
>> > --- Details ---
>> >
>> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >
>> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >
>> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >
>> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >
>> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >
>> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >
>> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >
>> > Please let us know if you have comments or concerns about this proposal.
>> >
>> > Thanks!
>> >
>> > --
>> > Paul Kirth
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
>
>
> --
> Paul Kirth
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

Roman


--
Paul Kirth

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:

>
>
>
> On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
>>
>> On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
>> <[hidden email]> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>> >>
>> >> Sounds like a useful tool.
>> >>
>> >
>> > Thanks! I certainly hope it will be.
>> >
>> >>
>> >> Could it be enough for clang to emit warnings in PGO mode when it
>> >> detects a mismatch between collected profile data and
>> >> __builtin_expect() ?
>> >>
>> >
>> > You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>> >
>> > However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>> >
>> > Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
>> Can those be simply reported as normal Remarks?
>> Those can later be visualized over the codebase via opt-viewer.
>>
>
> Could you clarify the context here for me? I'm not sure if you mean:
>
> 1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
> 2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
> 3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).
>
> If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.
>
> I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.

I think i'm failing to catch the subtle difference between 1. and 3.
I was indeed suggesting to *not* introduce a separate tool,
just emit the Remark (when some specific new diag group is not disabled)
when the PGO profile is specified/being used by clang.

>>
>> > When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>> >
>> > That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>> >
>> > Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>> >
>> >>
>> >> Michael
>> >>
>> >>
>> >> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> >> <[hidden email]>:
>> >> >
>> >> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >> >
>> >> > --- TLDR ---
>> >> > What are we proposing?
>> >> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >> >
>> >> > What is its purpose?
>> >> > * Identifying potentially problematic uses of __builtin_expect().
>> >> >
>> >> > Why is this important?
>> >> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >> >
>> >> > Where will the code live?
>> >> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >> >
>> >> > Open questions
>> >> > * What is the correct heuristic for determining a mismatch?
>> >> >
>> >> > Potential shortcomings
>> >> > * The approach is sensitive to both profiling input and configuration.
>> >> >
>> >> >
>> >> > --- Details ---
>> >> >
>> >> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >> >
>> >> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >> >
>> >> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >> >
>> >> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >> >
>> >> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >> >
>> >> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >> >
>> >> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >> >
>> >> > Please let us know if you have comments or concerns about this proposal.
>> >> >
>> >> > Thanks!
>> >> >
>> >> > --
>> >> > Paul Kirth
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > [hidden email]
>> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >
>> >
>> >
>> > --
>> > Paul Kirth
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>> Roman
>
>
>
> --
> Paul Kirth

Roman
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev


On Wed, Jul 24, 2019 at 5:42 PM Roman Lebedev <[hidden email]> wrote:
On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:
>
>
>
> On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
>>
>> On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
>> <[hidden email]> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>> >>
>> >> Sounds like a useful tool.
>> >>
>> >
>> > Thanks! I certainly hope it will be.
>> >
>> >>
>> >> Could it be enough for clang to emit warnings in PGO mode when it
>> >> detects a mismatch between collected profile data and
>> >> __builtin_expect() ?
>> >>
>> >
>> > You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>> >
>> > However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>> >
>> > Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
>> Can those be simply reported as normal Remarks?
>> Those can later be visualized over the codebase via opt-viewer.
>>
>
> Could you clarify the context here for me? I'm not sure if you mean:
>
> 1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
> 2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
> 3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).
>
> If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.
>
> I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.

I think i'm failing to catch the subtle difference between 1. and 3.
I was indeed suggesting to *not* introduce a separate tool,
just emit the Remark (when some specific new diag group is not disabled)
when the PGO profile is specified/being used by clang.


Sorry for the lack of clarity. 

In 1, in addition to the new flag in clang you still would have a standalone tool that emits warnings -- which I think is OK since it isn't the compiler itself and has a narrow, well defined scope. 

In 3, it's just as you describe: only an extension to the compiler based on Remarks, without any standalone tool. 

In my view, using Remarks here doesn't feel like the right tradeoff. They are tremendously useful for compiler developers, but I don't think the average user of clang is familiar with them, or should be. My exposure to Remarks has been exclusively as a diagnostic tool to understand the reasoning behind which optimizations have occurred or to debug something within the compiler. Maybe this is an aspect of the toolchain I'm unfamiliar with, but I can't think of another time we interface with developer using Remarks rather than directly giving them feedback. 

I also worry that if this information is only available in Remarks, then it will never be used, since it is kind of hidden. On the other hand, having a standalone tool fits well with existing workflows, and makes it easy for developers to try out. 

Other than my misgivings about issuing warnings based on a heuristic, is there any advantage to using Remarks over warnings? 

 
>>
>> > When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>> >
>> > That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>> >
>> > Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>> >
>> >>
>> >> Michael
>> >>
>> >>
>> >> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> >> <[hidden email]>:
>> >> >
>> >> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >> >
>> >> > --- TLDR ---
>> >> > What are we proposing?
>> >> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >> >
>> >> > What is its purpose?
>> >> > * Identifying potentially problematic uses of __builtin_expect().
>> >> >
>> >> > Why is this important?
>> >> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >> >
>> >> > Where will the code live?
>> >> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >> >
>> >> > Open questions
>> >> > * What is the correct heuristic for determining a mismatch?
>> >> >
>> >> > Potential shortcomings
>> >> > * The approach is sensitive to both profiling input and configuration.
>> >> >
>> >> >
>> >> > --- Details ---
>> >> >
>> >> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >> >
>> >> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >> >
>> >> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >> >
>> >> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >> >
>> >> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >> >
>> >> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >> >
>> >> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >> >
>> >> > Please let us know if you have comments or concerns about this proposal.
>> >> >
>> >> > Thanks!
>> >> >
>> >> > --
>> >> > Paul Kirth
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > [hidden email]
>> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >
>> >
>> >
>> > --
>> > Paul Kirth
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>> Roman
>
>
>
> --
> Paul Kirth

Roman


--
Paul Kirth

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev


On Jul 24, 2019, at 7:00 PM, Paul Kirth via cfe-dev <[hidden email]> wrote:



On Wed, Jul 24, 2019 at 5:42 PM Roman Lebedev <[hidden email]> wrote:
On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:

>
>
>
> On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
>>
>> On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
>> <[hidden email]> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>> >>
>> >> Sounds like a useful tool.
>> >>
>> >
>> > Thanks! I certainly hope it will be.
>> >
>> >>
>> >> Could it be enough for clang to emit warnings in PGO mode when it
>> >> detects a mismatch between collected profile data and
>> >> __builtin_expect() ?
>> >>
>> >
>> > You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>> >
>> > However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>> >
>> > Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
>> Can those be simply reported as normal Remarks?
>> Those can later be visualized over the codebase via opt-viewer.
>>
>
> Could you clarify the context here for me? I'm not sure if you mean:
>
> 1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
> 2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
> 3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).
>
> If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.
>
> I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.

I think i'm failing to catch the subtle difference between 1. and 3.
I was indeed suggesting to *not* introduce a separate tool,
just emit the Remark (when some specific new diag group is not disabled)
when the PGO profile is specified/being used by clang.


Sorry for the lack of clarity. 

In 1, in addition to the new flag in clang you still would have a standalone tool that emits warnings -- which I think is OK since it isn't the compiler itself and has a narrow, well defined scope. 

In 3, it's just as you describe: only an extension to the compiler based on Remarks, without any standalone tool. 

In my view, using Remarks here doesn't feel like the right tradeoff. They are tremendously useful for compiler developers, but I don't think the average user of clang is familiar with them, or should be.

As far as I'm aware, opt-remarks are actually intended for a general audience, not just compiler developers. I've cc'd some folks who've explored various ways to surface remarks / make them user-friendly. I actually think they're pretty user-friendly already -- e.g. there's support in tree for surfacing opt-remarks up via clang's DiagnosticEngine (i.e. they can look like warnings, get serialized, show up in an IDE, etc).



My exposure to Remarks has been exclusively as a diagnostic tool to understand the reasoning behind which optimizations have occurred or to debug something within the compiler. Maybe this is an aspect of the toolchain I'm unfamiliar with, but I can't think of another time we interface with developer using Remarks rather than directly giving them feedback. 

I also worry that if this information is only available in Remarks, then it will never be used, since it is kind of hidden. On the other hand, having a standalone tool fits well with existing workflows, and makes it easy for developers to try out.

If memory serves, remarks can be surfaced to the user by passing an extra compiler flag. That seems even easier than using a separate tool?

Also, istm that a new standalone tool will start off 'hidden' too.


Other than my misgivings about issuing warnings based on a heuristic, is there any advantage to using Remarks over warnings?

I think so. Using remarks can expose users to other important performance hints from the compiler, beyond what is/can-be surfaced by a standalone misexpect tool.

That's why it seems to me that the remarks infrastructure is the perfect fit for the hints you are describing.

best,
vedant

 
>>

>> > When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>> >
>> > That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>> >
>> > Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>> >
>> >>
>> >> Michael
>> >>
>> >>
>> >> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> >> <[hidden email]>:
>> >> >
>> >> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >> >
>> >> > --- TLDR ---
>> >> > What are we proposing?
>> >> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >> >
>> >> > What is its purpose?
>> >> > * Identifying potentially problematic uses of __builtin_expect().
>> >> >
>> >> > Why is this important?
>> >> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >> >
>> >> > Where will the code live?
>> >> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >> >
>> >> > Open questions
>> >> > * What is the correct heuristic for determining a mismatch?
>> >> >
>> >> > Potential shortcomings
>> >> > * The approach is sensitive to both profiling input and configuration.
>> >> >
>> >> >
>> >> > --- Details ---
>> >> >
>> >> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >> >
>> >> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >> >
>> >> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >> >
>> >> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >> >
>> >> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >> >
>> >> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >> >
>> >> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >> >
>> >> > Please let us know if you have comments or concerns about this proposal.
>> >> >
>> >> > Thanks!
>> >> >
>> >> > --
>> >> > Paul Kirth
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > [hidden email]
>> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >
>> >
>> >
>> > --
>> > Paul Kirth
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>> Roman
>
>
>
> --
> Paul Kirth

Roman


-- 
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
+Adam

> On Aug 15, 2019, at 5:59 PM, Vedant Kumar via cfe-dev <[hidden email]> wrote:
>
>
>
>> On Jul 24, 2019, at 7:00 PM, Paul Kirth via cfe-dev <[hidden email]> wrote:
>>
>>
>>
>> On Wed, Jul 24, 2019 at 5:42 PM Roman Lebedev <[hidden email]> wrote:
>> On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
>> >>
>> >> On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
>> >> <[hidden email]> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>> >> >>
>> >> >> Sounds like a useful tool.
>> >> >>
>> >> >
>> >> > Thanks! I certainly hope it will be.
>> >> >
>> >> >>
>> >> >> Could it be enough for clang to emit warnings in PGO mode when it
>> >> >> detects a mismatch between collected profile data and
>> >> >> __builtin_expect() ?
>> >> >>
>> >> >
>> >> > You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>> >> >
>> >> > However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>> >> >
>> >> > Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
>> >> Can those be simply reported as normal Remarks?
>> >> Those can later be visualized over the codebase via opt-viewer.
>> >>
>> >
>> > Could you clarify the context here for me? I'm not sure if you mean:
>> >
>> > 1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
>> > 2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
>> > 3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).
>> >
>> > If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.
>> >
>> > I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.
>>
>> I think i'm failing to catch the subtle difference between 1. and 3.
>> I was indeed suggesting to *not* introduce a separate tool,
>> just emit the Remark (when some specific new diag group is not disabled)
>> when the PGO profile is specified/being used by clang.
>>
>>
>> Sorry for the lack of clarity.
>>
>> In 1, in addition to the new flag in clang you still would have a standalone tool that emits warnings -- which I think is OK since it isn't the compiler itself and has a narrow, well defined scope.
>>
>> In 3, it's just as you describe: only an extension to the compiler based on Remarks, without any standalone tool.
>>
>> In my view, using Remarks here doesn't feel like the right tradeoff. They are tremendously useful for compiler developers, but I don't think the average user of clang is familiar with them, or should be.
>
> As far as I'm aware, opt-remarks are actually intended for a general audience, not just compiler developers. I've cc'd some folks who've explored various ways to surface remarks / make them user-friendly. I actually think they're pretty user-friendly already -- e.g. there's support in tree for surfacing opt-remarks up via clang's DiagnosticEngine (i.e. they can look like warnings, get serialized, show up in an IDE, etc).
>
>
>
>> My exposure to Remarks has been exclusively as a diagnostic tool to understand the reasoning behind which optimizations have occurred or to debug something within the compiler. Maybe this is an aspect of the toolchain I'm unfamiliar with, but I can't think of another time we interface with developer using Remarks rather than directly giving them feedback.
>>
>> I also worry that if this information is only available in Remarks, then it will never be used, since it is kind of hidden. On the other hand, having a standalone tool fits well with existing workflows, and makes it easy for developers to try out.
>
> If memory serves, remarks can be surfaced to the user by passing an extra compiler flag. That seems even easier than using a separate tool?

Some documentation about how remarks are surfaced is available here: http://llvm.org/docs/Remarks.html. I tend to agree with Vedant here.

>
> Also, istm that a new standalone tool will start off 'hidden' too.
>
>
>> Other than my misgivings about issuing warnings based on a heuristic, is there any advantage to using Remarks over warnings?
>
> I think so. Using remarks can expose users to other important performance hints from the compiler, beyond what is/can-be surfaced by a standalone misexpect tool.
>
> That's why it seems to me that the remarks infrastructure is the perfect fit for the hints you are describing.

I agree here as well. With remarks, you can see how the “misexpect” affected optimizations by seeing other remarks describing missed or passed optimizations.

Is there anything lacking in the remarks “infrastructure” that makes them not fit (or harder to use) for your use case?

Thanks,


Francis

>
> best,
> vedant
>
>>  
>> >>
>> >> > When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>> >> >
>> >> > That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>> >> >
>> >> > Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>> >> >
>> >> >>
>> >> >> Michael
>> >> >>
>> >> >>
>> >> >> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> >> >> <[hidden email]>:
>> >> >> >
>> >> >> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >> >> >
>> >> >> > --- TLDR ---
>> >> >> > What are we proposing?
>> >> >> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >> >> >
>> >> >> > What is its purpose?
>> >> >> > * Identifying potentially problematic uses of __builtin_expect().
>> >> >> >
>> >> >> > Why is this important?
>> >> >> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >> >> >
>> >> >> > Where will the code live?
>> >> >> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >> >> >
>> >> >> > Open questions
>> >> >> > * What is the correct heuristic for determining a mismatch?
>> >> >> >
>> >> >> > Potential shortcomings
>> >> >> > * The approach is sensitive to both profiling input and configuration.
>> >> >> >
>> >> >> >
>> >> >> > --- Details ---
>> >> >> >
>> >> >> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >> >> >
>> >> >> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >> >> >
>> >> >> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >> >> >
>> >> >> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >> >> >
>> >> >> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >> >> >
>> >> >> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >> >> >
>> >> >> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >> >> >
>> >> >> > Please let us know if you have comments or concerns about this proposal.
>> >> >> >
>> >> >> > Thanks!
>> >> >> >
>> >> >> > --
>> >> >> > Paul Kirth
>> >> >> > _______________________________________________
>> >> >> > cfe-dev mailing list
>> >> >> > [hidden email]
>> >> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Paul Kirth
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > [hidden email]
>> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >>
>> >> Roman
>> >
>> >
>> >
>> > --
>> > Paul Kirth
>>
>> Roman
>>
>>
>> --
>> Paul Kirth
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev


On Aug 16, 2019, at 8:51 AM, Francis Visoiu Mistrih <[hidden email]> wrote:

+Adam

On Aug 15, 2019, at 5:59 PM, Vedant Kumar via cfe-dev <[hidden email]> wrote:



On Jul 24, 2019, at 7:00 PM, Paul Kirth via cfe-dev <[hidden email]> wrote:



On Wed, Jul 24, 2019 at 5:42 PM Roman Lebedev <[hidden email]> wrote:
On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:



On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:

On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
<[hidden email]> wrote:



On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:

Sounds like a useful tool.


Thanks! I certainly hope it will be.


Could it be enough for clang to emit warnings in PGO mode when it
detects a mismatch between collected profile data and
__builtin_expect() ?


You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.

However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.

Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
Can those be simply reported as normal Remarks?
Those can later be visualized over the codebase via opt-viewer.


Could you clarify the context here for me? I'm not sure if you mean:

1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).

If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.

I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.

I think i'm failing to catch the subtle difference between 1. and 3.
I was indeed suggesting to *not* introduce a separate tool,
just emit the Remark (when some specific new diag group is not disabled)
when the PGO profile is specified/being used by clang.


Sorry for the lack of clarity. 

In 1, in addition to the new flag in clang you still would have a standalone tool that emits warnings -- which I think is OK since it isn't the compiler itself and has a narrow, well defined scope. 

In 3, it's just as you describe: only an extension to the compiler based on Remarks, without any standalone tool. 

In my view, using Remarks here doesn't feel like the right tradeoff. They are tremendously useful for compiler developers, but I don't think the average user of clang is familiar with them, or should be.

As far as I'm aware, opt-remarks are actually intended for a general audience, not just compiler developers. I've cc'd some folks who've explored various ways to surface remarks / make them user-friendly. I actually think they're pretty user-friendly already -- e.g. there's support in tree for surfacing opt-remarks up via clang's DiagnosticEngine (i.e. they can look like warnings, get serialized, show up in an IDE, etc).



My exposure to Remarks has been exclusively as a diagnostic tool to understand the reasoning behind which optimizations have occurred or to debug something within the compiler. Maybe this is an aspect of the toolchain I'm unfamiliar with, but I can't think of another time we interface with developer using Remarks rather than directly giving them feedback. 

I also worry that if this information is only available in Remarks, then it will never be used, since it is kind of hidden. On the other hand, having a standalone tool fits well with existing workflows, and makes it easy for developers to try out.

If memory serves, remarks can be surfaced to the user by passing an extra compiler flag. That seems even easier than using a separate tool?

Some documentation about how remarks are surfaced is available here: http://llvm.org/docs/Remarks.html. I tend to agree with Vedant here.


Also, istm that a new standalone tool will start off 'hidden' too.


Other than my misgivings about issuing warnings based on a heuristic, is there any advantage to using Remarks over warnings?

I think so. Using remarks can expose users to other important performance hints from the compiler, beyond what is/can-be surfaced by a standalone misexpect tool.

That's why it seems to me that the remarks infrastructure is the perfect fit for the hints you are describing.

I agree here as well. With remarks, you can see how the “misexpect” affected optimizations by seeing other remarks describing missed or passed optimizations.

+1

Essentially you want to display this remark not only when someone is *specifically* asking for “misexpect” missed optimizations but also when the user is asking for *any* potentially missed optimizations.  When looking for this “specific” remark you can just filter the remarks.  When looking for *any* missed optimizations the opt-viewer should also display this remark along with the other missed optimization remarks.

The added benefit is that remarks already contain a “hotness” attribute when using PGO so you could order your misexpect remarks but relevance.  

Adam


Is there anything lacking in the remarks “infrastructure” that makes them not fit (or harder to use) for your use case?

Thanks,

 
Francis


best,
vedant



When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.

That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.

Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.


Michael


Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
<[hidden email]>:

We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().

--- TLDR ---
What are we proposing?
* Adding a new tool, clang-mispredict, to the LLVM project.

What is its purpose?
* Identifying potentially problematic uses of __builtin_expect().

Why is this important?
* __builtin_expect() has a high performance cost when it is used incorrectly.

Where will the code live?
* clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.

Open questions
* What is the correct heuristic for determining a mismatch?

Potential shortcomings
* The approach is sensitive to both profiling input and configuration.


--- Details ---

We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.

Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.

Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).

We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.

The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.

Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.

Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.

Please let us know if you have comments or concerns about this proposal.

Thanks!

--
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



--
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

Roman



--
Paul Kirth

Roman


-- 
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev


On Thu, Aug 15, 2019 at 5:59 PM Vedant Kumar <[hidden email]> wrote:


On Jul 24, 2019, at 7:00 PM, Paul Kirth via cfe-dev <[hidden email]> wrote:



On Wed, Jul 24, 2019 at 5:42 PM Roman Lebedev <[hidden email]> wrote:
On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:

>
>
>
> On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
>>
>> On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
>> <[hidden email]> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>> >>
>> >> Sounds like a useful tool.
>> >>
>> >
>> > Thanks! I certainly hope it will be.
>> >
>> >>
>> >> Could it be enough for clang to emit warnings in PGO mode when it
>> >> detects a mismatch between collected profile data and
>> >> __builtin_expect() ?
>> >>
>> >
>> > You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>> >
>> > However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>> >
>> > Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
>> Can those be simply reported as normal Remarks?
>> Those can later be visualized over the codebase via opt-viewer.
>>
>
> Could you clarify the context here for me? I'm not sure if you mean:
>
> 1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
> 2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
> 3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).
>
> If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.
>
> I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.

I think i'm failing to catch the subtle difference between 1. and 3.
I was indeed suggesting to *not* introduce a separate tool,
just emit the Remark (when some specific new diag group is not disabled)
when the PGO profile is specified/being used by clang.


Sorry for the lack of clarity. 

In 1, in addition to the new flag in clang you still would have a standalone tool that emits warnings -- which I think is OK since it isn't the compiler itself and has a narrow, well defined scope. 

In 3, it's just as you describe: only an extension to the compiler based on Remarks, without any standalone tool. 

In my view, using Remarks here doesn't feel like the right tradeoff. They are tremendously useful for compiler developers, but I don't think the average user of clang is familiar with them, or should be.

As far as I'm aware, opt-remarks are actually intended for a general audience, not just compiler developers. I've cc'd some folks who've explored various ways to surface remarks / make them user-friendly. I actually think they're pretty user-friendly already -- e.g. there's support in tree for surfacing opt-remarks up via clang's DiagnosticEngine (i.e. they can look like warnings, get serialized, show up in an IDE, etc).



Thanks for the feedback and looping in some others into the discussion. I've definitely been exploring the Remarks infrastructure as the discussion has developed.
 

My exposure to Remarks has been exclusively as a diagnostic tool to understand the reasoning behind which optimizations have occurred or to debug something within the compiler. Maybe this is an aspect of the toolchain I'm unfamiliar with, but I can't think of another time we interface with developer using Remarks rather than directly giving them feedback. 

I also worry that if this information is only available in Remarks, then it will never be used, since it is kind of hidden. On the other hand, having a standalone tool fits well with existing workflows, and makes it easy for developers to try out.

If memory serves, remarks can be surfaced to the user by passing an extra compiler flag. That seems even easier than using a separate tool?

Also, istm that a new standalone tool will start off 'hidden' too.


Fair point.
 

Other than my misgivings about issuing warnings based on a heuristic, is there any advantage to using Remarks over warnings?

I think so. Using remarks can expose users to other important performance hints from the compiler, beyond what is/can-be surfaced by a standalone misexpect tool.


That's true, but I still think there is a benefit to a standalone tool that you can just run over a codebase through the compiler commands database. It also has the benefit of not requiring any changes to the build other than a way to generate a representative profile. I think that provides users an easy way to check many configurations with only minimal effort. The richer compiler infrastructure seems to be more well suited to deeper investigations.
 
That's why it seems to me that the remarks infrastructure is the perfect fit for the hints you are describing.
 

Maybe we can have both remarks and an optional warning? I agree that there is a benefit to exposing this diagnostic as part of the remarks infrastructure, and that it is a good fit for misexpect in many ways. I think it would still be nice to give users a way to also turn this into a warning. 
 
best,
vedant

 
>>

>> > When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>> >
>> > That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>> >
>> > Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>> >
>> >>
>> >> Michael
>> >>
>> >>
>> >> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> >> <[hidden email]>:
>> >> >
>> >> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >> >
>> >> > --- TLDR ---
>> >> > What are we proposing?
>> >> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >> >
>> >> > What is its purpose?
>> >> > * Identifying potentially problematic uses of __builtin_expect().
>> >> >
>> >> > Why is this important?
>> >> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >> >
>> >> > Where will the code live?
>> >> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >> >
>> >> > Open questions
>> >> > * What is the correct heuristic for determining a mismatch?
>> >> >
>> >> > Potential shortcomings
>> >> > * The approach is sensitive to both profiling input and configuration.
>> >> >
>> >> >
>> >> > --- Details ---
>> >> >
>> >> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >> >
>> >> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >> >
>> >> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >> >
>> >> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >> >
>> >> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >> >
>> >> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >> >
>> >> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >> >
>> >> > Please let us know if you have comments or concerns about this proposal.
>> >> >
>> >> > Thanks!
>> >> >
>> >> > --
>> >> > Paul Kirth
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > [hidden email]
>> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >
>> >
>> >
>> > --
>> > Paul Kirth
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>> Roman
>
>
>
> --
> Paul Kirth

Roman


-- 
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



--
Paul Kirth

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev


On Fri, Aug 16, 2019 at 8:52 AM Francis Visoiu Mistrih <[hidden email]> wrote:
+Adam

> On Aug 15, 2019, at 5:59 PM, Vedant Kumar via cfe-dev <[hidden email]> wrote:
>
>
>
>> On Jul 24, 2019, at 7:00 PM, Paul Kirth via cfe-dev <[hidden email]> wrote:
>>
>>
>>
>> On Wed, Jul 24, 2019 at 5:42 PM Roman Lebedev <[hidden email]> wrote:
>> On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
>> >>
>> >> On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
>> >> <[hidden email]> wrote:
>> >> >
>> >> >
>> >> >
>> >> > On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>> >> >>
>> >> >> Sounds like a useful tool.
>> >> >>
>> >> >
>> >> > Thanks! I certainly hope it will be.
>> >> >
>> >> >>
>> >> >> Could it be enough for clang to emit warnings in PGO mode when it
>> >> >> detects a mismatch between collected profile data and
>> >> >> __builtin_expect() ?
>> >> >>
>> >> >
>> >> > You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>> >> >
>> >> > However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>> >> >
>> >> > Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
>> >> Can those be simply reported as normal Remarks?
>> >> Those can later be visualized over the codebase via opt-viewer.
>> >>
>> >
>> > Could you clarify the context here for me? I'm not sure if you mean:
>> >
>> > 1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
>> > 2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
>> > 3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).
>> >
>> > If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.
>> >
>> > I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.
>>
>> I think i'm failing to catch the subtle difference between 1. and 3.
>> I was indeed suggesting to *not* introduce a separate tool,
>> just emit the Remark (when some specific new diag group is not disabled)
>> when the PGO profile is specified/being used by clang.
>>
>>
>> Sorry for the lack of clarity.
>>
>> In 1, in addition to the new flag in clang you still would have a standalone tool that emits warnings -- which I think is OK since it isn't the compiler itself and has a narrow, well defined scope.
>>
>> In 3, it's just as you describe: only an extension to the compiler based on Remarks, without any standalone tool.
>>
>> In my view, using Remarks here doesn't feel like the right tradeoff. They are tremendously useful for compiler developers, but I don't think the average user of clang is familiar with them, or should be.
>
> As far as I'm aware, opt-remarks are actually intended for a general audience, not just compiler developers. I've cc'd some folks who've explored various ways to surface remarks / make them user-friendly. I actually think they're pretty user-friendly already -- e.g. there's support in tree for surfacing opt-remarks up via clang's DiagnosticEngine (i.e. they can look like warnings, get serialized, show up in an IDE, etc).
>
>
>
>> My exposure to Remarks has been exclusively as a diagnostic tool to understand the reasoning behind which optimizations have occurred or to debug something within the compiler. Maybe this is an aspect of the toolchain I'm unfamiliar with, but I can't think of another time we interface with developer using Remarks rather than directly giving them feedback.
>>
>> I also worry that if this information is only available in Remarks, then it will never be used, since it is kind of hidden. On the other hand, having a standalone tool fits well with existing workflows, and makes it easy for developers to try out.
>
> If memory serves, remarks can be surfaced to the user by passing an extra compiler flag. That seems even easier than using a separate tool?

Some documentation about how remarks are surfaced is available here: http://llvm.org/docs/Remarks.html. I tend to agree with Vedant here.

>
> Also, istm that a new standalone tool will start off 'hidden' too.
>
>
>> Other than my misgivings about issuing warnings based on a heuristic, is there any advantage to using Remarks over warnings?
>
> I think so. Using remarks can expose users to other important performance hints from the compiler, beyond what is/can-be surfaced by a standalone misexpect tool.
>
> That's why it seems to me that the remarks infrastructure is the perfect fit for the hints you are describing.

I agree here as well. With remarks, you can see how the “misexpect” affected optimizations by seeing other remarks describing missed or passed optimizations.

Is there anything lacking in the remarks “infrastructure” that makes them not fit (or harder to use) for your use case?


In the case of a standalone tool, I think issuing warnings is the right approach. Warning a developer about the impact of something that they wrote vs a decision that the compiler made also played a role in my preference for a warning. 

To me the remark didn't seem serious enough. From the discussion, I think that I simply had a poor understanding of the role remarks play within the compiler. 

I already suggested this to Vedant, but maybe a good approach is to support remarks and a warning.

 
Thanks,


Francis

>
> best,
> vedant
>
>> 
>> >>
>> >> > When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>> >> >
>> >> > That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>> >> >
>> >> > Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>> >> >
>> >> >>
>> >> >> Michael
>> >> >>
>> >> >>
>> >> >> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> >> >> <[hidden email]>:
>> >> >> >
>> >> >> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >> >> >
>> >> >> > --- TLDR ---
>> >> >> > What are we proposing?
>> >> >> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >> >> >
>> >> >> > What is its purpose?
>> >> >> > * Identifying potentially problematic uses of __builtin_expect().
>> >> >> >
>> >> >> > Why is this important?
>> >> >> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >> >> >
>> >> >> > Where will the code live?
>> >> >> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >> >> >
>> >> >> > Open questions
>> >> >> > * What is the correct heuristic for determining a mismatch?
>> >> >> >
>> >> >> > Potential shortcomings
>> >> >> > * The approach is sensitive to both profiling input and configuration.
>> >> >> >
>> >> >> >
>> >> >> > --- Details ---
>> >> >> >
>> >> >> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >> >> >
>> >> >> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >> >> >
>> >> >> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >> >> >
>> >> >> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >> >> >
>> >> >> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >> >> >
>> >> >> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >> >> >
>> >> >> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >> >> >
>> >> >> > Please let us know if you have comments or concerns about this proposal.
>> >> >> >
>> >> >> > Thanks!
>> >> >> >
>> >> >> > --
>> >> >> > Paul Kirth
>> >> >> > _______________________________________________
>> >> >> > cfe-dev mailing list
>> >> >> > [hidden email]
>> >> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Paul Kirth
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > [hidden email]
>> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >>
>> >> Roman
>> >
>> >
>> >
>> > --
>> > Paul Kirth
>>
>> Roman
>>
>>
>> --
>> Paul Kirth
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



--
Paul Kirth

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev
FYI

It is possible to dump branch probabilities as optimization remarks via an internal option.  Not convenient as the tool, but user can use it to inspect the code easily.  The following example shows that builtin_expect is wrongly used -- it should expect '1', instead of '0'.

// command line:
clang -O2 -fprofile-use=./t.profdata -Rpass=pgo-instrumentation -mllvm -pgo-emit-branch-prob foo.c -c

// output:
foo.c:3:9: remark: sgt_i32_Const is true with probability : 0x71eb851f / 0x80000000 = 89.00% (total count : 100) [-Rpass=pgo-instrumentation]
    if (__builtin_expect(n > 10, 0))
        ^
foo.c:11:5: remark: ult_i32_Const is true with probability : 0x7ebb907a / 0x80000000 = 99.01% (total count : 101) [-Rpass=pgo-instrumentation]
    for (int i = 0; i < 100; i++) {

    ^

// foo.c:
int g1, g2;
__attribute__((noinline)) void foo(int n) {
    if (__builtin_expect(n > 10, 0))
      g1++;
    else
       g2++;
}

int main() {
    int i;
    for (int i = 0; i < 100; i++) {
       foo(i);
    }
    return 0;
}




On Tue, Jul 23, 2019 at 7:05 PM Paul Kirth via cfe-dev <[hidden email]> wrote:
We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().  
 
--- TLDR ---
What are we proposing?
* Adding a new tool, clang-mispredict, to the LLVM project.
 
What is its purpose?
* Identifying potentially problematic uses of __builtin_expect().
 
Why is this important?
* __builtin_expect() has a high performance cost when it is used incorrectly.
 
Where will the code live?
* clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
 
Open questions
* What is the correct heuristic for determining a mismatch?
 
Potential shortcomings
* The approach is sensitive to both profiling input and configuration.
 
 
--- Details ---
 
We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
 
Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
 
Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
 
We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
 
The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
 
Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
 
Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
 
Please let us know if you have comments or concerns about this proposal.

Thanks!

--
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: RFC: Clang-Misexpect Proposal

Kristóf Umann via cfe-dev
In reply to this post by Kristóf Umann via cfe-dev

On Aug 16, 2019, at 11:01 AM, Paul Kirth via cfe-dev <[hidden email]> wrote:



On Thu, Aug 15, 2019 at 5:59 PM Vedant Kumar <[hidden email]> wrote:


On Jul 24, 2019, at 7:00 PM, Paul Kirth via cfe-dev <[hidden email]> wrote:



On Wed, Jul 24, 2019 at 5:42 PM Roman Lebedev <[hidden email]> wrote:
On Thu, Jul 25, 2019 at 3:24 AM Paul Kirth <[hidden email]> wrote:

>
>
>
> On Wed, Jul 24, 2019 at 3:14 PM Roman Lebedev <[hidden email]> wrote:
>>
>> On Thu, Jul 25, 2019 at 1:06 AM Paul Kirth via cfe-dev
>> <[hidden email]> wrote:
>> >
>> >
>> >
>> > On Wed, Jul 24, 2019 at 6:31 AM Michael Kruse <[hidden email]> wrote:
>> >>
>> >> Sounds like a useful tool.
>> >>
>> >
>> > Thanks! I certainly hope it will be.
>> >
>> >>
>> >> Could it be enough for clang to emit warnings in PGO mode when it
>> >> detects a mismatch between collected profile data and
>> >> __builtin_expect() ?
>> >>
>> >
>> > You are right: it might be sufficient to simply gate these warnings behind a flag in the PGO pipeline. I think there is a good argument for doing it this way. In fact, it's how my prototype works right now, so we have certainly considered this option.
>> >
>> > However, I think it will still be desirable to have a standalone tool that doesn't require the full compilation pipeline. My understanding was that clang-tools are often aimed at this exact use-case: i.e. a custom driver and partial compilation pipeline.
>> >
>> > Another important point is that compiler warnings are, by and large, accurate. Our approach to identifying these mismatches is subject to some imprecision due to a deficiency in the collected profile.
>> Can those be simply reported as normal Remarks?
>> Those can later be visualized over the codebase via opt-viewer.
>>
>
> Could you clarify the context here for me? I'm not sure if you mean:
>
> 1. using remarks in place of warnings in the PGO pipeline, but not a standalone tool.
> 2. using remarks in place of warnings in both a standalone tool and in the PGO pipeline.
> 3. using remarks in place of warnings in the PGO pipeline, with no standalone tool (so only a compiler flag).
>
> If we're only talking about case 1, then I think that could be a good compromise w.r.t using warnings in the compiler.
>
> I'm not sure I see an advantage to using remarks in a standalone tool(scenario 2), or as the sole method of reporting(scenario 3). I'm not even sure if that was what you meant, but I'm happy to hear more about the approach if I have missed something.

I think i'm failing to catch the subtle difference between 1. and 3.
I was indeed suggesting to *not* introduce a separate tool,
just emit the Remark (when some specific new diag group is not disabled)
when the PGO profile is specified/being used by clang.


Sorry for the lack of clarity. 

In 1, in addition to the new flag in clang you still would have a standalone tool that emits warnings -- which I think is OK since it isn't the compiler itself and has a narrow, well defined scope. 

In 3, it's just as you describe: only an extension to the compiler based on Remarks, without any standalone tool. 

In my view, using Remarks here doesn't feel like the right tradeoff. They are tremendously useful for compiler developers, but I don't think the average user of clang is familiar with them, or should be.

As far as I'm aware, opt-remarks are actually intended for a general audience, not just compiler developers. I've cc'd some folks who've explored various ways to surface remarks / make them user-friendly. I actually think they're pretty user-friendly already -- e.g. there's support in tree for surfacing opt-remarks up via clang's DiagnosticEngine (i.e. they can look like warnings, get serialized, show up in an IDE, etc).



Thanks for the feedback and looping in some others into the discussion. I've definitely been exploring the Remarks infrastructure as the discussion has developed.
 

My exposure to Remarks has been exclusively as a diagnostic tool to understand the reasoning behind which optimizations have occurred or to debug something within the compiler. Maybe this is an aspect of the toolchain I'm unfamiliar with, but I can't think of another time we interface with developer using Remarks rather than directly giving them feedback. 

I also worry that if this information is only available in Remarks, then it will never be used, since it is kind of hidden. On the other hand, having a standalone tool fits well with existing workflows, and makes it easy for developers to try out.

If memory serves, remarks can be surfaced to the user by passing an extra compiler flag. That seems even easier than using a separate tool?

Also, istm that a new standalone tool will start off 'hidden' too.


Fair point.
 

Other than my misgivings about issuing warnings based on a heuristic, is there any advantage to using Remarks over warnings?

I think so. Using remarks can expose users to other important performance hints from the compiler, beyond what is/can-be surfaced by a standalone misexpect tool.


That's true, but I still think there is a benefit to a standalone tool that you can just run over a codebase through the compiler commands database. It also has the benefit of not requiring any changes to the build other than a way to generate a representative profile.

Sure, but it seems like adding a cflag is about as burdensome as running a separate tool, if not less so?


I think that provides users an easy way to check many configurations with only minimal effort. The richer compiler infrastructure seems to be more well suited to deeper investigations.

Not sure I follow.

 
That's why it seems to me that the remarks infrastructure is the perfect fit for the hints you are describing.
 

Maybe we can have both remarks and an optional warning?

Sure. One way to do this is to add a -Wpass facility (I’m sure there’s a better name :) to turn misexpect remarks into warnings.

My two cents is that it would be really great to avoid introducing a standalone tool, with all the concomitant maintenance overhead (yet another multi-MB binary in the distribution..) just to surface a warning.

best,
vedant 

I agree that there is a benefit to exposing this diagnostic as part of the remarks infrastructure, and that it is a good fit for misexpect in many ways. I think it would still be nice to give users a way to also turn this into a warning. 




 
best,
vedant

 
>>

>> > When we issue a warning in clang-misexpect, it means one of two things: (i) your annotation is incorrect, or (ii) your profile data is insufficient. While it at least signals to the developer that investigation is warranted, it is a somewhat unsatisfactory feedback. I think our best effort warnings are a better fit for a clang-tool because of that. To me, warnings in the main compiler have a feeling of more significance and a higher burden of correctness. For something less precise or not generally applicable, a clang-tool appears to be the right place to implement this. This seems to be consistent with existing tools, like clang-tidy, that enable more warnings/diagnostics than are available in the main compiler.
>> >
>> > That being said, we're happy to take feedback from the community. So if the general consensus is that this should be part of the PGO pipeline, then we can rescope our proposal to only add a new flag to clang.
>> >
>> > Or maybe the right thing to do is to have both? I'm not exactly sure, which is one of the reasons we made this RFC.
>> >
>> >>
>> >> Michael
>> >>
>> >>
>> >> Am Di., 23. Juli 2019 um 22:06 Uhr schrieb Paul Kirth via cfe-dev
>> >> <[hidden email]>:
>> >> >
>> >> > We would like to propose adding a new clang based tool, clang-mispredict, for identifying potentially incorrect uses of __builtin_expect().
>> >> >
>> >> > --- TLDR ---
>> >> > What are we proposing?
>> >> > * Adding a new tool, clang-mispredict, to the LLVM project.
>> >> >
>> >> > What is its purpose?
>> >> > * Identifying potentially problematic uses of __builtin_expect().
>> >> >
>> >> > Why is this important?
>> >> > * __builtin_expect() has a high performance cost when it is used incorrectly.
>> >> >
>> >> > Where will the code live?
>> >> > * clang-mispredict will live in clang-tools-extra, and there will be some additions to clang.
>> >> >
>> >> > Open questions
>> >> > * What is the correct heuristic for determining a mismatch?
>> >> >
>> >> > Potential shortcomings
>> >> > * The approach is sensitive to both profiling input and configuration.
>> >> >
>> >> >
>> >> > --- Details ---
>> >> >
>> >> > We plant to build a new tool, clang-mispredict, on top of Clang’s PGO infrastructure. This tool will issue warnings about the use of _builtin_expect() when collected PGO profile data shows a mismatch with the use of __builtin_expect(). Existing solutions for validating these annotations generally follow this approach: leverage dynamic profiling to validate existing annotations. This is the approach used in the Linux kernel, for example. Unfortunately, existing solutions seem to be custom efforts specific to a particular code base. Supporting this in the LLVM ecosystem gives developers a general way to check the use of _builtin_expect() without creating custom instrumentation.
>> >> >
>> >> > Additionally, if _builtin_expect() is used incorrectly, then developers may notice a performance regression when switching to the new PM. This happens because the New PM follows performance annotations more closely than the legacy PM, and is therefore more likely to try and aggressively optimize these cases. Our proposed tool can help developers narrow their focus to potentially problematic areas during the transition.
>> >> >
>> >> > Finally, it may also be beneficial to suggest annotations when profiling suggests it would be beneficial. We consider this a desirable property not only from a performance standpoint, but also for identifying potential bugs (both in the profiling corpus and within the codebase).
>> >> >
>> >> > We think having clang-mispredict behave similar to clang-tidy is good approach. We can use a compile commands database in conjunction with profiling data to drive our validation tool. More concretely, we plan to use Clang’s existing PGO infrastructure to emit warnings where branch weights are assigned, but stop compilation before the IR is passed to the backend. This largely amounts to checking if the profile counters are outside of certain thresholds for 'hot' and 'cold' code. Similar to clang tidy, we plan to support suppressing warnings within the codebase through use of comment strings.
>> >> >
>> >> > The most obvious question about this approach is how to quantify what is ‘hot’ or ‘cold’. Initially, we plan to use the thresholds already present in LLVM; however, we are happy to use any suitable heuristic or empirically derived threshold. Input from the community about the correct heuristic are most welcome.
>> >> >
>> >> > Our strategy does have some shortcomings. The usefulness of the warnings are directly related to how representative the profiling input actually was when compared to its normal use. If the profile collected is not representative of typical use, then the warnings may not reflect the ground truth. Program and build configuration can also dramatically change which paths will be executed, thus affecting the quality of the profile data and the generated warnings.
>> >> >
>> >> > Ultimately, we are proposing adding a new standalone tool that takes a compile commands database and an instrumentation profile to emit warnings in a clang-tidy fashion.
>> >> >
>> >> > Please let us know if you have comments or concerns about this proposal.
>> >> >
>> >> > Thanks!
>> >> >
>> >> > --
>> >> > Paul Kirth
>> >> > _______________________________________________
>> >> > cfe-dev mailing list
>> >> > [hidden email]
>> >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> >
>> >
>> >
>> > --
>> > Paul Kirth
>> > _______________________________________________
>> > cfe-dev mailing list
>> > [hidden email]
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>>
>> Roman
>
>
>
> --
> Paul Kirth

Roman


-- 
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



--
Paul Kirth
_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev