Re: [PATCH] PR rtl-optimization/64081: Enable RTL loop unrolling for duplicated exit blocks and back edges.

[David Edelsohn <dje dot gcc at gmail dot com>]

On Fri, Feb 5, 2016 at 3:27 PM, Jeff Law <law@redhat.com> wrote:
> On 02/05/2016 06:43 AM, Alexander Fomin wrote:
>>
>> Hi!
>>
>> Some kind of this patch was submitted about a year ago by Igor
>> Zamyatin. It's an attempt to fix PR rtl-optimization/64081 by enabling
>> RTL loop unrolling for duplicated exit blocks and back edges.
>>
>> At the time it caused AIX bootstrap failure, but now it's OK according
>> to David's testing. I've also bootstrapped and regtested it on
>> x86_64-linux-gnu.
>>
>> Is it still OK for trunk now, or you consider this v7 stuff?
>> Anyway, it's a regression.
>>
>> Thanks,
>> Alexander
>> ---
>> gcc/
>>
>>         PR rtl-optimization/64081
>>         * loop-iv.c (def_pred_latch_p): New function.
>>         (latch_dominating_def): Allow specific cases with non-single
>>         definitions.
>>         (iv_get_reaching_def): Likewise.
>>         (check_complex_exit_p): New function.
>>         (check_simple_exit): Use check_complex_exit_p to allow certain
>> cases
>>         with exits not executing on any iteration.
>>
>> gcc/testsuite
>>
>>         PR rtl-optimization/64081
>>         * gcc.dg/pr64081.c: New test.
>
> Normally I'd say that if it was approved before, then it's still good to go
> since there haven't been major conceptual changes in this code since the
> patch was originally written and now.
>
> However, in this instance the patch had been reported to cause problems on
> AIX, problems that we can't reproduce now -- which makes me want to be more
> cautious.  Was it a problem with the patch, or some other latent issue -- we
> don't know at this point.
>
> So I think the way to go is to apply this patch on top of r219827 where it
> caused the AIX failure.  Then bootstrap on aix and determine the root cause
> of of the AIX bootstrap failure.  If it's this patch, then update the patch
> as needed.  If the patch is just exposing a latent bug elsewhere, we should
> evaluate whether or not that latent but has been fixed or not before
> applying this fix to the trunk.
>
> It's considerably more work, but ISTM it's the right thing to do.

I'm on the fence about this patch.  I definitely don't think that it
should be merged for GCC 6.

If the patch were to be proposed during Stage 1 for GCC 7 and had not
caused bootstrap problems for AIX, no one would have any question.

The problem is we don't know if the patch exposed a latent bug that
independently was fixed after the patch was reverted or if the patch
still contains a bug that has been rendered latent by another change.

Another approach to track down the cause would be to bisect which
patch fixed the bootstrap failure if the patch had not been reverted.

I agree with Jeff that a statement that "the original patch magically
works now" is not a good justification for merging it -- at least not
for GCC 6.

Thanks, David