Re: [PATCH]Partially fix PR61529, bound basic block frequency

[Teresa Johnson <tejohnson@google.com>]

On Thu, Nov 6, 2014 at 7:09 AM, Renlin Li <renlin.li@arm.com> wrote:
>
> Hi Jeff,
>
> Test case has been added. With the patch, both x86_64-unknown-linux-gnu and
> aarch64-none-elf compile the test case successfully.
>
> Okay to commit?
>
>
> On 04/11/14 21:59, Jeff Law wrote:
>>
>> On 11/03/14 08:29, Renlin Li wrote:
>>>
>>> On 29/10/14 12:42, Teresa Johnson wrote:
>>>>
>>>> Hi Renlin,
>>>>
>>>> Are the incoming edge counts or probabilities insane in this case? I
>>>> guess the patch is ok if we need to do this to handle those incoming
>>>> insanitiles. But I can't approve patches myself.
>>>
>>> Not really, it's just a little bigger than the limit.
>>>
>>> For this particular test case, ABC is a threaded path.
>>> B is the fallthrough basic block of A, D is a basic block split from B
>>> (used to be a self loop). A, B and D have roughly the same frequency (
>>> 8281, 9100, 8281).
>>> When calculating the path_in_freq, frequencies from AB and DB edges are
>>> accumulated, and the final result is large than BB_FREQ_MAX.
>>>
>>>
>>>             A
>>> 100% |
>>>             |      9%
>>> ------>B---------->C
>>> |         |
>>> |100%| 91%
>>> |         |
>>> --------D

The frequencies look insane given these probabilities. If most of the
execution stays in the loop then B should have a much higher frequency
than A.

>>>
>>>
>>>
>>> There are 2 suspicious points:
>>> 1, The BD edge is not correctly guessed at the profile stage. However,
>>> anyway it's heuristic, so I don't think, it's here the problem starts.
>>> 2, The BD edge is not eliminated before jump threading. But the jump
>>> threading pass will analysis the condition jump statement in B block (In
>>> this particular case, the BD probability should be zero), and makes the
>>> decision to thread it.
>>>
>>> Later in the dom pass, the BD edge is indeed removed.
>>
>> Can you add a testcase please?  With a testcase, this patch is OK for
>> the trunk.
>>
>> jeff
>>
>
> x86_64-unknown-linux-gnu bootstrap and regression test have been done, no
> new issue.
> aarch64-none-elf toolchain has been test on the model. No new regression.
>
> gcc/ChangeLog:
>
> 2014-11-06  Renlin Li  <Renlin.Li@arm.com>
>     PR middle-end/61529
>     * tree-ssa-threadupdate.c (compute_path_counts): Bound path_in_freq.

Please add a comment that this is needed due to insane incoming frequencies.

>
> gcc/testsuite/ChangeLog:
>
> 2014-11-06  Renlin Li  <Renlin.Li@arm.com>
>     PR middle-end/61529
>     * gcc.dg/pr61529.c: New.

The 'b' variable is uninitialized. Also, 'd' and 'a' may end up
uninitialized depending on the initial value of 'b'. Please initialize
these.

Thanks,
Teresa


-- 
Teresa Johnson | Software Engineer | tejohnson@google.com | 408-460-2413