This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: FDO usability patch -- correct insane profile data


I reverted the original patch and added the assertion instead.

Thanks,

David

On Sun, Apr 17, 2011 at 2:31 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Adding assertion sounds good to me -- the only problem is that problem
>> like this hard to be triggered during testing, thus making assertion
>> less useful as it can be. Is it better to add the assertion with
>> checking is enabled while still keeping the correction code?
> Hi,
> since we speak about code that has minimal to none testing coverage in our
> automates testers (we don't really test anything multithreaded or with mismatching
> profiles), I don't think gcc_checking_assert would make any difference.
>
> Since we don't really want the future bug to stay around unnoticed (since it
> would resolut in silent misupdates of profiles), we are only shooting to make
> analysis easier so next time someone trips around the scenario he won't see
> symptomatic message from cgraph verifier.
> So I would go for normal assert and comment about nature of the bug.
>
> Honza
>>
>> Thanks,
>>
>> David
>>
>> On Sun, Apr 17, 2011 at 11:53 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > Hi,
>> > hmm, looks we both run into same issue and developed different fixes.
>> >>
>> >> Good point. The change above was added based on 4.4.3 where the sum
>> >> computation has no capping like above. Yes, with the current capping,
>> >> the negative value won't result. However, it is still good to guard
>> >> the scale independent of the implementation of ipcp_compute_node_scale
>> >> -- it may change and break silently (the comment of the function says
>> >> the implementation is wrong...)
>> >
>> > Well, if we want to add check in anticipation that we will introduce bug few
>> > lines up, I think we could just add gcc_assert (scale < BASE); with an comment
>> > explaining that ipcp_compute_node_scale must give results in range even when
>> > profile is not flow consistent.
>> >
>> > Since we need to propagate the scales across callgraph (at least for the
>> > correct implementation of the function), masking bug in ipcp_compute_node_scale
>> > would make us to propagate the nonsenses further and silently producing
>> > unnecesarily aplified insanity.
>> >
>> > I would pre-approve patch reverting the current change and adding the assert
>> > with comment.
>> >
>> > Honza
>> >>
>> >> Thanks,
>> >>
>> >> David
>> >>
>> >>
>> >> >
>> >> > Honza
>> >> >
>> >
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]