On Fri, Nov 8, 2013 at 11:21 AM, Rong Xu <x...@google.com> wrote:
> On Fri, Nov 8, 2013 at 11:02 AM, Xinliang David Li <davi...@google.com> wrote:
>> On Fri, Nov 8, 2013 at 10:48 AM, Rong Xu <x...@google.com> wrote:
>>> On Fri, Nov 8, 2013 at 10:22 AM, Xinliang David Li <davi...@google.com>
>>> wrote:
>>>> in libgcov-driver.c
>>>>
>>>>> /* Flag when the profile has already been dumped via __gcov_dump(). */
>>>>> static int gcov_dump_complete;
>>>>
>>>>> inline void
>>>>> set_gcov_dump_complete (void)
>>>>>{
>>>> > gcov_dump_complete = 1;
>>>>>}
>>>>
>>>>>inline void
>>>>>reset_gcov_dump_complete (void)
>>>>>{
>>>>> gcov_dump_complete = 0;
>>>>>}
>>>>
>>>>
>>>> These should be moved to libgcov-interface.c. Is there reason to not
>>>> mark them as static?
>>>
>>> gcov_dump_complete is used in gcov_exit() which is in
>>> libgcov-driver.c, but it's set in __gcov_dump and __gcov_reset, both
>>> in libgcov-interface.c.
>>> I want gcov_dump_complete a static. So I have to add to global
>>> functions that accessible from libgcov-interface.c.
>>> Another choice is to move __gcov_dump and __gcov_reset to
>>> libgcov-driver.c and that will simplify the code.
>>>
>>
>> Ok then you should remove the 'inline' keyword for the set and reset
>> functions and keep them in libgcov-driver.c.
>
> Will remove 'inline' keyword.
>
>>
>>>>
>>>> in libgcov-profiler.c, line 142
>>>>
>>>>> #if defined(HAVE_CC_TLS) && !defined (USE_EMUTLS)
>>>>> __thread
>>>>
>>>> trailing whilte space.
>>>>
>>>
>>> Will fix.
>>>
>>>>
>>>>> || (VTABLE_USES_DESCRIPTORS && __gcov_indirect_call_callee
>>>>> && *(void **) cur_func == *(void **) __gcov_indirect_call_callee))
>>>>
>>>> trailing white space.
>>>>
>>>
>>> Will fix.
>>>
>>>>
>>>> in libgcov-merge.c
>>>>
>>>> 1) I don't think you need in_mem flag. For in memory merge, the source !=
>>>> NULL.
>>>> 2) document the new source and weight parameter -- especially the weight.
>>>
>>> Will do.
>>>
>>>> 3) How do you deal with integer overflow ?
>>>
>>> This is good question. gcvo_type is (typically) long long. I thought
>>> the count value will not be nearly close to the max of long long.
>>> (We did see overflow in compiler, but that's because of the scaling --
>>> some of the scaling factors are really large.)
>>>
>>
>> Why not passing weight as a scaled PERCENT (as branch prob) for the
>> source? THis way, the merge tool does not need to do scaling twice.
>>
>
> there are two weights, so unless you have two weights in the merge_add
> functions, you have to
> traverse the list twice. Do you mean pass addition parameters?
>
> Currently, merge tools is done this way:
> merge (p1, p2, w1, w2)
> first pass: merge_add (p1, p1, w1)
> second pass: merge_add (p1, p2, w2)
>
Only need to pass the normalized the weight (in percent) for one
profile source -- say norm_w2 : w2/(w1+w2), and the merge function can
do this in one pass as norm_w1 = 1-norm_w2.
> I initially had both weights in merge_add function. but later I found that
> to deal with mis-match profile, I need to find out the gcov_info in p1
> but not p2, (they need to multiply by w1 only).
> So I choose the above structure.
>
> Another thing about the PERCENT is the inaccuracy for floating points.
This is how profile update work in profile-use compilation -- so that
should not be a big problem.
Before you fix this, I'd like to hear what Honza and other reviewers think.
thanks,
David
>
> I have the scaling function in rewrite sub-command mainly for debug purpose:
> I merge the same profile with a weight 1:9.
> Then I scale the result profile by 0.1. I was expecting identical
> profile as the source. But due to floating point inaccuracy, some
> counters are off slightly.
>
>
>> David
>>
>>
>>
>>>>
>>>> David
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Thu, Nov 7, 2013 at 3:34 PM, Rong Xu <x...@google.com> wrote:
>>>>> On Thu, Nov 7, 2013 at 9:40 AM, Joseph S. Myers <jos...@codesourcery.com>
>>>>> wrote:
>>>>>> On Thu, 7 Nov 2013, Rong Xu wrote:
>>>>>>
>>>>>>> Thanks Joseph for these detailed comments/suggestions.
>>>>>>> The fixed patch is attached to this email.
>>>>>>> The only thing left out is the Texinfo manual. Do you mean this tool
>>>>>>> should have its it's own texi file in gcc/doc?
>>>>>>
>>>>>> Its own texi file, probably included as a chapter by gcc.texi (like
>>>>>> gcov.texi is).
>>>>>
>>>>> OK. will add this in.
>>>>>
>>>>> My last patch that changes the command handling is actually broken.
>>>>> Please ignore that patch and use the one in this email.
>>>>>
>>>>> Also did some minor adjust of the code in libgcov.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Rong
>>>>>
>>>>>
>>>>>>
>>>>>> --
>>>>>> Joseph S. Myers
>>>>>> jos...@codesourcery.com
