Message ID | 5695436D.4000003@mentor.com |
---|---|
State | New |
Headers | show |
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. > 2. > > -XFAIL: gcc.dg/graphite/scop-pr66980.c scan-tree-dump-times graphite "number > of SCoPs: 1" 1 > +XPASS: gcc.dg/graphite/scop-pr66980.c scan-tree-dump-times graphite "number > of SCoPs: 1" 1 > > AFAIU, this is not a real regression, but the testcase needs to be updated. > I'm not sure how. Sebastian, perhaps you have an idea there? It looks like simply removing the xfail might be ok. But the comment in the testcase doesn't suggest its dependency analysis fault that the situation is not handled so I'd like Sebastian to chime in (who also should know the dependence code very well). Thanks, Richard.
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? Thanks, - Tom
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.
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. > After r232659 (the fix for pr68692), the ICE no longer occurs. >> 2. >> >> -XFAIL: gcc.dg/graphite/scop-pr66980.c scan-tree-dump-times graphite "number >> of SCoPs: 1" 1 >> +XPASS: gcc.dg/graphite/scop-pr66980.c scan-tree-dump-times graphite "number >> of SCoPs: 1" 1 >> >> AFAIU, this is not a real regression, but the testcase needs to be updated. >> I'm not sure how. Sebastian, perhaps you have an idea there? > > It looks like simply removing the xfail might be ok. But the comment in > the testcase doesn't suggest its dependency analysis fault that the > situation is not handled so I'd like Sebastian to chime in (who also > should know the dependence code very well). > Sebastian, Ping on the xfail -> xpass issue mentioned above. I'd like to commit this ( https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00762.html ) patch. I'm currently retesting using r232712 as baseline. Thanks, - Tom
On 23/01/16 18:39, Sebastian Pop wrote: > diff --git a/gcc/tree-data-ref.c b/gcc/tree-data-ref.c > index a40f40d..7ff5db7 100644 > --- a/gcc/tree-data-ref.c > +++ b/gcc/tree-data-ref.c > @@ -1023,6 +1023,10 @@ dr_analyze_indices (struct data_reference *dr, > loop_p nest, loop_p loop) > build_int_cst (reference_alias_ptr_type (ref), 0)); > } > > + /* Ensure that DR_NUM_DIMENSIONS (dr) != 0. */ > + if (access_fns == vNULL) > + access_fns.safe_push (integer_zero_node); > + > DR_BASE_OBJECT (dr) = ref; > DR_ACCESS_FNS (dr) = access_fns; > } > > This code adds a data reference with an access function 0 for all data > references > that cannot be analyzed by the data reference analysis. Let's move > this check under > the check for DECL_P (ref) and this fixes the check in graphite: > > diff --git a/gcc/tree-data-ref.c b/gcc/tree-data-ref.c > index a40f40d..862589b 100644 > --- a/gcc/tree-data-ref.c > +++ b/gcc/tree-data-ref.c > @@ -1018,12 +1018,15 @@ dr_analyze_indices (struct data_reference *dr, > loop_p nest, loop_p loop) > else if (DECL_P (ref)) > { > /* Canonicalize DR_BASE_OBJECT to MEM_REF form. */ > ref = build2 (MEM_REF, TREE_TYPE (ref), > build_fold_addr_expr (ref), > build_int_cst (reference_alias_ptr_type (ref), 0)); > + > + if (access_fns == vNULL) > + access_fns.safe_push (integer_zero_node); > } > > DR_BASE_OBJECT (dr) = ref; > DR_ACCESS_FNS (dr) = access_fns; > } > > Ok with this change. > Hi Sebastian, That was my original patch, and Richard commented: 'I think avoiding a NULL access_fns is ok but it should be done unconditionally, not only for the DECL_P case'. In order words, he asked me to do the exact opposite of the change you now propose. Thanks, - Tom
On Sat, Jan 23, 2016 at 12:28 PM, Tom de Vries <Tom_deVries@mentor.com> wrote: > That was my original patch, and Richard commented: 'I think avoiding a NULL > access_fns is ok but it should be done unconditionally, not only for the > DECL_P case'. In order words, he asked me to do the exact opposite of the > change you now propose. > In the case of a DECL_P it is correct to say that it has an access function of 0. In the graphite testcase it is not correct to say that the access function for a given data reference is zero: we only initialize access_fns in the case of a polynomial chrec: if (TREE_CODE (ref) == MEM_REF) { op = TREE_OPERAND (ref, 0); access_fn = analyze_scalar_evolution (loop, op); access_fn = instantiate_scev (before_loop, loop, access_fn); if (TREE_CODE (access_fn) == POLYNOMIAL_CHREC) { [...] access_fns.safe_push (access_fn); } } In all other cases we may not have a representation of the access functions. It is incorrect to initialize to "A[0]" all those data references that cannot be analyzed. If needed, instead of returning vNULL, one could initialize the vector to empty: if (access_fns == vNULL) access_fns.create (0); and that would be correct, though it would not teach the dependence analysis how to deal with the global variable access function in pr69110. I think the fix is to add the zero subscript only for DECL_P (ref). Sebastian
On January 23, 2016 7:44:23 PM GMT+01:00, Sebastian Pop <sebpop@gmail.com> wrote: >On Sat, Jan 23, 2016 at 12:28 PM, Tom de Vries <Tom_deVries@mentor.com> >wrote: >> That was my original patch, and Richard commented: 'I think avoiding >a NULL >> access_fns is ok but it should be done unconditionally, not only for >the >> DECL_P case'. In order words, he asked me to do the exact opposite of >the >> change you now propose. >> > >In the case of a DECL_P it is correct to say that it has an access >function of 0. >In the graphite testcase it is not correct to say that the access >function for a given data reference is zero: >we only initialize access_fns in the case of a polynomial chrec: > > if (TREE_CODE (ref) == MEM_REF) > { > op = TREE_OPERAND (ref, 0); > access_fn = analyze_scalar_evolution (loop, op); > access_fn = instantiate_scev (before_loop, loop, access_fn); > if (TREE_CODE (access_fn) == POLYNOMIAL_CHREC) > { >[...] > access_fns.safe_push (access_fn); > } > } > >In all other cases we may not have a representation of the access >functions. >It is incorrect to initialize to "A[0]" all those data references that >cannot be analyzed. But does it matter as the base will not be equal with one that can be analyzed? >If needed, instead of returning vNULL, one could initialize the vector >to empty: > >if (access_fns == vNULL) > access_fns.create (0); > >and that would be correct, though it would not teach the dependence >analysis >how to deal with the global variable access function in pr69110. >I think the fix is to add the zero subscript only for DECL_P (ref). > >Sebastian
From 24dfdb5a8a536203ad159bcbeaee6931be032f32 Mon Sep 17 00:00:00 2001 From: Tom de Vries <tom@codesourcery.com> Date: Tue, 12 Jan 2016 01:45:11 +0100 Subject: [PATCH] Don't return NULL access_fns in dr_analyze_indices 2016-01-12 Tom de Vries <tom@codesourcery.com> * tree-data-ref.c (dr_analyze_indices): Don't return NULL access_fns. * gcc.dg/autopar/pr69110.c: New test. * testsuite/libgomp.c/pr69110.c: New test. --- gcc/testsuite/gcc.dg/autopar/pr69110.c | 19 +++++++++++++++++++ gcc/tree-data-ref.c | 4 ++++ libgomp/testsuite/libgomp.c/pr69110.c | 26 ++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/autopar/pr69110.c create mode 100644 libgomp/testsuite/libgomp.c/pr69110.c diff --git a/gcc/testsuite/gcc.dg/autopar/pr69110.c b/gcc/testsuite/gcc.dg/autopar/pr69110.c new file mode 100644 index 0000000..e236015 --- /dev/null +++ b/gcc/testsuite/gcc.dg/autopar/pr69110.c @@ -0,0 +1,19 @@ +/* { dg-do compile } */ +/* { dg-options "-O1 -ftree-parallelize-loops=2 -fno-tree-loop-im -fdump-tree-parloops-details" } */ + +#define N 1000 + +unsigned int i = 0; + +void +foo (void) +{ + unsigned int z; + for (z = 0; z < N; ++z) + ++i; +} + +/* { dg-final { scan-tree-dump-times "SUCCESS: may be parallelized" 0 "parloops" } } */ +/* { dg-final { scan-tree-dump-times "FAILED: data dependencies exist across iterations" 1 "parloops" } } */ + + diff --git a/gcc/tree-data-ref.c b/gcc/tree-data-ref.c index a40f40d..7ff5db7 100644 --- a/gcc/tree-data-ref.c +++ b/gcc/tree-data-ref.c @@ -1023,6 +1023,10 @@ dr_analyze_indices (struct data_reference *dr, loop_p nest, loop_p loop) build_int_cst (reference_alias_ptr_type (ref), 0)); } + /* Ensure that DR_NUM_DIMENSIONS (dr) != 0. */ + if (access_fns == vNULL) + access_fns.safe_push (integer_zero_node); + DR_BASE_OBJECT (dr) = ref; DR_ACCESS_FNS (dr) = access_fns; } diff --git a/libgomp/testsuite/libgomp.c/pr69110.c b/libgomp/testsuite/libgomp.c/pr69110.c new file mode 100644 index 0000000..0d9e5ca --- /dev/null +++ b/libgomp/testsuite/libgomp.c/pr69110.c @@ -0,0 +1,26 @@ +/* { dg-do run } */ +/* { dg-options "-ftree-parallelize-loops=2 -O1 -fno-tree-loop-im" } */ + +#define N 1000 + +unsigned int i = 0; + +static void __attribute__((noinline, noclone)) +foo (void) +{ + unsigned int z; + for (z = 0; z < N; ++z) + ++i; +} + +extern void abort (void); + +int +main (void) +{ + foo (); + if (i != N) + abort (); + + return 0; +} -- 1.9.1