Re: [GOOGLE] Report the difference between profiled and guessed or annotated branch probabilities.

[Xinliang David Li <davidxl at google dot com>]

and for google/gcc4_9 too.

David

On Tue, Jul 1, 2014 at 1:16 PM, Dehao Chen <dehao@google.com> wrote:
> OK for google-4_8 after testing.
>
> Thanks,
> Dehao
>
> On Tue, Jul 1, 2014 at 1:04 PM, Yi Yang <ahyangyi@google.com> wrote:
>> Per offline discussion,
>> * do not export function start line number. Instead, hash branch
>> offset and discriminator into the "function_hash" (renamed to just
>> "hash" to clarify)
>> * protect all operations about the new EDGE_PREDICTED_BY_EXPECT flag
>> with flag_check_branch_annotation.
>>
>> On Mon, Jun 30, 2014 at 5:42 PM, Yi Yang <ahyangyi@google.com> wrote:
>>> Fixed a style error (missing a space before a left parenthesis)
>>>
>>>
>>> On Mon, Jun 30, 2014 at 5:41 PM, Yi Yang <ahyangyi@google.com> wrote:
>>>> Done.
>>>>
>>>> On Mon, Jun 30, 2014 at 5:20 PM, Dehao Chen <dehao@google.com> wrote:
>>>>> For get_locus_information, can you cal get_inline_stack and directly use its
>>>>> output to get the function name instead?
>>>>>
>>>>> Dehao
>>>>>
>>>>>
>>>>> On Mon, Jun 30, 2014 at 4:47 PM, Yi Yang <ahyangyi@google.com> wrote:
>>>>>>
>>>>>> Removed fill_invalid_locus_information. Change the function call to a
>>>>>> return statement.
>>>>>>
>>>>>> On Mon, Jun 30, 2014 at 4:42 PM, Dehao Chen <dehao@google.com> wrote:
>>>>>> > There is no need for fill_invalid_locus_information, just initialize
>>>>>> > every field to 0, and if it's unknown location, no need to output this
>>>>>> > line.
>>>>>> >
>>>>>> > Dehao
>>>>>> >
>>>>>> > On Mon, Jun 30, 2014 at 4:26 PM, Yi Yang <ahyangyi@google.com> wrote:
>>>>>> >> Instead of storing percentages of the branch probabilities, store them
>>>>>> >> times REG_BR_PROB_BASE.
>>>>>> >>
>>>>>> >> On Mon, Jun 30, 2014 at 3:26 PM, Yi Yang <ahyangyi@google.com> wrote:
>>>>>> >>> Fixed. (outputting only the integer percentage)
>>>>>> >>>
>>>>>> >>> On Mon, Jun 30, 2014 at 2:20 PM, Yi Yang <ahyangyi@google.com> wrote:
>>>>>> >>>> This is intermediate result, which is meant to be consumed by further
>>>>>> >>>> post-processing. For this reason I'd prefer to put a number without
>>>>>> >>>> that percentage sign.
>>>>>> >>>>
>>>>>> >>>> I'd just output (int)(probability*100000000+0.5). Does this look good
>>>>>> >>>> for you? Or maybe change that to 1000000 since six digits are more
>>>>>> >>>> than enough. I don't see a reason to intentionally drop precision
>>>>>> >>>> though.
>>>>>> >>>>
>>>>>> >>>> Note that for the actual probability, the best way to store it is to
>>>>>> >>>> store the edge count, since the probability is just
>>>>>> >>>> edge_count/bb_count. But this causes disparity in the formats of the
>>>>>> >>>> two probabilities.
>>>>>> >>>>
>>>>>> >>>> On Mon, Jun 30, 2014 at 2:12 PM, Dehao Chen <dehao@google.com> wrote:
>>>>>> >>>>> Let's use %d to replace %f (manual conversion, let's do xx%).
>>>>>> >>>>>
>>>>>> >>>>> Dehao
>>>>>> >>>>>
>>>>>> >>>>> On Mon, Jun 30, 2014 at 2:06 PM, Yi Yang <ahyangyi@google.com>
>>>>>> >>>>> wrote:
>>>>>> >>>>>> Fixed.
>>>>>> >>>>>>
>>>>>> >>>>>> Also, I spotted some warnings caused by me using "%lf"s in
>>>>>> >>>>>> snprintf().
>>>>>> >>>>>> I changed these to "%f" and tested.
>>>>>> >>>>>>
>>>>>> >>>>>>
>>>>>> >>>>>> On Mon, Jun 30, 2014 at 1:49 PM, Dehao Chen <dehao@google.com>
>>>>>> >>>>>> wrote:
>>>>>> >>>>>>> You don't need extra space to store file name in
>>>>>> >>>>>>> locus_information_t.
>>>>>> >>>>>>> Use pointer instead.
>>>>>> >>>>>>>
>>>>>> >>>>>>> Dehao
>>>>>> >>>>>>>
>>>>>> >>>>>>>
>>>>>> >>>>>>> On Mon, Jun 30, 2014 at 1:36 PM, Yi Yang <ahyangyi@google.com>
>>>>>> >>>>>>> wrote:
>>>>>> >>>>>>>>
>>>>>> >>>>>>>> I refactored the code and added comments. A bug (prematurely
>>>>>> >>>>>>>> breaking
>>>>>> >>>>>>>> from a loop) was fixed during the refactoring.
>>>>>> >>>>>>>>
>>>>>> >>>>>>>> (My last mail was wrongly set to HTML instead of plain text. I
>>>>>> >>>>>>>> apologize for that.)
>>>>>> >>>>>>>>
>>>>>> >>>>>>>> 2014-06-30  Yi Yang  <ahyangyi@google.com>
>>>>>> >>>>>>>>
>>>>>> >>>>>>>>     * auto-profile.c (get_locus_information)
>>>>>> >>>>>>>>     (fill_invalid_locus_information,
>>>>>> >>>>>>>> record_branch_prediction_results)
>>>>>> >>>>>>>>     (afdo_calculate_branch_prob, afdo_annotate_cfg): Main
>>>>>> >>>>>>>> comparison and
>>>>>> >>>>>>>>     reporting logic.
>>>>>> >>>>>>>>     * cfg-flags.def (PREDICTED_BY_EXPECT): Add an extra flag
>>>>>> >>>>>>>> representing
>>>>>> >>>>>>>>     an edge's probability is predicted by annotations.
>>>>>> >>>>>>>>     * predict.c (combine_predictions_for_bb): Set up the extra
>>>>>> >>>>>>>> flag on an
>>>>>> >>>>>>>>     edge when appropriate.
>>>>>> >>>>>>>>     * common.opt (fcheck-branch-annotation)
>>>>>> >>>>>>>>     (fcheck-branch-annotation-threshold=): Add an extra GCC
>>>>>> >>>>>>>> option to turn
>>>>>> >>>>>>>>     on report
>>>>>> >>>>>>>>
>>>>>> >>>>>>>> On Fri, Jun 27, 2014 at 3:20 PM, Xinliang David Li
>>>>>> >>>>>>>> <davidxl@google.com> wrote:
>>>>>> >>>>>>>> > Hi Yi,
>>>>>> >>>>>>>> >
>>>>>> >>>>>>>> > 1) please add comments before new functions as documentation --
>>>>>> >>>>>>>> > follow
>>>>>> >>>>>>>> > the coding style guideline
>>>>>> >>>>>>>> > 2) missing documenation on the new flags (pointed out by
>>>>>> >>>>>>>> > Gerald)
>>>>>> >>>>>>>> > 3) Please refactor the check code in afdo_calculate_branch_prob
>>>>>> >>>>>>>> > into a
>>>>>> >>>>>>>> > helper function
>>>>>> >>>>>>>> >
>>>>>> >>>>>>>> > 4) the change log is not needed for google branches, but if
>>>>>> >>>>>>>> > provided,
>>>>>> >>>>>>>> > the format should follow the style guide (e.g, function name in
>>>>>> >>>>>>>> > () ).
>>>>>> >>>>>>>> >
>>>>>> >>>>>>>> > David
>>>>>> >>>>>>>> >
>>>>>> >>>>>>>> >
>>>>>> >>>>>>>> > On Fri, Jun 27, 2014 at 11:07 AM, Yi Yang <ahyangyi@google.com>
>>>>>> >>>>>>>> > wrote:
>>>>>> >>>>>>>> >> Hi,
>>>>>> >>>>>>>> >>
>>>>>> >>>>>>>> >> This patch adds an option. When the option is enabled, GCC
>>>>>> >>>>>>>> >> will add a
>>>>>> >>>>>>>> >> record about it in an elf section called
>>>>>> >>>>>>>> >> ".gnu.switches.text.branch.annotation" for every branch.
>>>>>> >>>>>>>> >>
>>>>>> >>>>>>>> >> gcc/
>>>>>> >>>>>>>> >>
>>>>>> >>>>>>>> >> 2014-06-27 Yi Yang <ahyangyi@google.com>
>>>>>> >>>>>>>> >>
>>>>>> >>>>>>>> >>         * auto-profile.c: Main comparison and reporting logic.
>>>>>> >>>>>>>> >>         * cfg-flags.def: Add an extra flag representing an
>>>>>> >>>>>>>> >> edge's
>>>>>> >>>>>>>> >> probability is predicted by annotations.
>>>>>> >>>>>>>> >>         * predict.c: Set up the extra flag on an edge when
>>>>>> >>>>>>>> >> appropriate.
>>>>>> >>>>>>>> >>         * common.opt: Add an extra GCC option to turn on this
>>>>>> >>>>>>>> >> report mechanism
>>>>>
>>>>>