diff mbox

[PR69110] Don't return NULL access_fns in dr_analyze_indices

Message ID 5695436D.4000003@mentor.com
State New
Headers show

Commit Message

Tom de Vries Jan. 12, 2016, 6:18 p.m. UTC 
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?

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?

Thanks,
- Tom

Comments

Richard Biener Jan. 13, 2016, 8:42 a.m. UTC | #1 
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.
Tom de Vries Jan. 15, 2016, 10:15 a.m. UTC | #2 
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
Richard Biener Jan. 15, 2016, 10:18 a.m. UTC | #3 
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.
Tom de Vries Jan. 21, 2016, 11:47 p.m. UTC | #4 
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
Tom de Vries Jan. 23, 2016, 6:28 p.m. UTC | #5 
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
Sebastian Pop Jan. 23, 2016, 6:44 p.m. UTC | #6 
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
Richard Biener Jan. 24, 2016, 8:04 a.m. UTC | #7 
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
diff mbox

Patch

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