Re: [PATCH, PR69110] Don't return NULL access_fns in dr_analyze_indices

[Richard Biener <rguenther@suse.de>]

On Fri, 15 Jan 2016, Tom de Vries wrote:

> On 13/01/16 09:42, Richard Biener wrote:
> > On Tue, 12 Jan 2016, Tom de Vries wrote:
> > 
> > > >On 12/01/16 14:04, Richard Biener wrote:
> > > > > >On Tue, 12 Jan 2016, Tom de Vries wrote:
> > > > > >
> > > > > > > >On 12/01/16 12:22, Richard Biener wrote:
> > > > > > > > > >Doesnt' the same issue apply to
> > > > > > > > > >
> > > > > > > > > > > > > >unsigned int *p;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >static void __attribute__((noinline, noclone))
> > > > > > > > > > > > > >foo (void)
> > > > > > > > > > > > > >{
> > > > > > > > > > > > > >    unsigned int z;
> > > > > > > > > > > > > >
> > > > > > > > > > > > > >    for (z = 0; z < N; ++z)
> > > > > > > > > > > > > >      ++(*p);
> > > > > > > > > > > > > >}
> > > > > > > > > >thus when we have a MEM_REF[p_1]?  SCEV will not analyze
> > > > > > > > > >its evolution to a POLYNOMIAL_CHREC and thus access_fns will
> > > > > > > > > >be NULL again.
> > > > > > > > > >
> > > > > > > >
> > > > > > > >I didn't manage to trigger this scenario, though I could probably
> > > > > make it
> > > > > > > >happen by modifying ftree-loop-im to work in one case (the load
> > > > > of the
> > > > > > > >value
> > > > > > > >of p) but not the other (the *p load and store).
> > > > > > > >
> > > > > > > > > >I think avoiding a NULL access_fns is ok but it should be
> > > > > > done
> > > > > > > > > >unconditionally, not only for the DECL_P case.
> > > > > > > >
> > > > > > > >Ok, I'll retest and commit this patch.
> > > > > >
> > > > > >Please add a comment as well.
> > > >
> > > >Patch updated with comment.
> > > >
> > > >During testing however, I ran into two testsuite regressions:
> > > >
> > > >1.
> > > >
> > > >-PASS: gfortran.dg/graphite/pr39516.f   -O  (test for excess errors)
> > > >+FAIL: gfortran.dg/graphite/pr39516.f   -O  (internal compiler error)
> > > >+FAIL: gfortran.dg/graphite/pr39516.f   -O  (test for excess errors)
> > > >
> > > >AFAIU, this is a duplicate of PR68976.
> > > >
> > > >Should I wait with committing the patch until PR68976 is fixed?
> > Yes - we shouldn't introduce new testsuite regressions willingly at this
> > point.
> > 
> 
> I've looked in more detail at both PR68976 and the pr39516.f regression using
> this patch, and I now think they are independent.
> 
> Furthermore, I managed to reproduce the pr39516.f regression without the patch
> (filed as PR69292 - '[graphite] ICE with -floop-nest-optimize').
> 
> So, AFAIU, committing this patch does not introduce a new type of ICE, but it
> makes it more likely that you run into it.
> 
> Does that still mean we need to wait with committing, and first fix PR69292?

Yes as it introduces a testsuite regression.

Richard.