: [Bug tree-optimization/61917] [4.9/5 Regression] ICE on valid code at -O3 on x86_64-linux-gnu in vectorizable_reduction, at tree-vect-loop.c:4913
diff mbox

Message ID CAFiYyc0abfFKJqzzg_eX=z5FPkduvpwPyK0upLu8LoKef4Hiyg@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Feb. 25, 2015, 11:35 a.m. UTC
On Wed, Feb 25, 2015 at 12:05 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
> 2015-02-25 11:57 GMT+01:00 Richard Biener <richard.guenther@gmail.com>:
>> On Wed, Feb 25, 2015 at 11:06 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>> Hello,
>>>
>>> ChangeLog
>>>
>>> 2015-02-25  Kai Tietz  <ktietz@redhat.com>
>>>
>>>     PR tree-optimization/61917
>>>     * tree-vect-loop.c (vectorizable_reduction): Allow
>>>     vect_internal_def without reduction to exit graceful.
>>>
>>> ChagneLog testsuite/
>>>
>>> 2015-02-25  Kai Tietz  <ktietz@redhat.com>
>>>
>>>     PR tree-optimization/61917
>>>     * gcc.dg/vect/vect-pr61917.c: New file.
>>>
>>> Tested for x86_64-unkown-linux.  Ok for apply?
>>
>> It doesn't make much sense to fail here as said in the comment
>> because of patterns if the actual case isn't a pattern.  Also
>> the patch causing this made vect_external_def possible, so
>> why does this affect vect_internal_def here?
>>
>> It may paper over the issue - but clearly the fix is bogus.
>
> Well, actually I don't see any good reason for failing here at all,
> but I assumed that author wanted to get by it some missed cases.
> AFAIU the code we actually don't need/want to assert here either on
> internal (where it is pretty likely no pattern present), and also on
> externals, too.
> I would be fine to remove this assert here completely, but I thought
> it might be of interest to see that orig_stmt isn't NULL for nested
> case

I agree that the code is overusing asserts but the path you bail out
on is bogus.  Fact is that we somehow detected a reduction here
(via special-casing of minus I guess) but fail to properly handle it
later.  In fact we assert that we end up with a PHI node in the definition
but would ICE there as well.  Thus a better patch would be


Richard.

> Kai
>
>> Richard.
>>
>>> Regards,
>>> Kai
>>>
>>> Index: tree-vect-loop.c
>>> ===================================================================
>>> --- tree-vect-loop.c    (Revision 220958)
>>> +++ tree-vect-loop.c    (Arbeitskopie)
>>> @@ -4990,7 +4990,7 @@ vectorizable_reduction (gimple stmt, gimple_stmt_i
>>>        /* For pattern recognized stmts, orig_stmt might be a reduction,
>>>       but some helper statements for the pattern might not, or
>>>       might be COND_EXPRs with reduction uses in the condition.  */
>>> -      gcc_assert (orig_stmt);
>>> +      gcc_assert (orig_stmt || dt == vect_internal_def);
>>>        return false;
>>>      }
>>>    if (!found_nested_cycle_def)
>>> Index: gcc/gcc/testsuite/gcc.dg/vect/vect-pr61917.c
>>> ===================================================================
>>> --- /dev/null
>>> +++ gcc/gcc/testsuite/gcc.dg/vect/vect-pr61917.c
>>> @@ -0,0 +1,13 @@
>>> +/* { dg-do compile } */
>>> +/* { dg-additional-options "-O3" } */
>>> +
>>> +int a, b, c, d;
>>> +
>>> +int
>>> +fn1 ()
>>> +{
>>> +  for (; c; c++)
>>> +    for (b = 0; b < 2; b++)
>>> +      d = a - d;
>>> +  return d;
>>> +}

Comments

Kai Tietz Feb. 25, 2015, 12:17 p.m. UTC | #1
2015-02-25 12:35 GMT+01:00 Richard Biener <richard.guenther@gmail.com>:
> On Wed, Feb 25, 2015 at 12:05 PM, Kai Tietz <ktietz70@googlemail.com> wrote:
>> 2015-02-25 11:57 GMT+01:00 Richard Biener <richard.guenther@gmail.com>:
>>> On Wed, Feb 25, 2015 at 11:06 AM, Kai Tietz <ktietz70@googlemail.com> wrote:
>>>> Hello,
>>>>
>>>> ChangeLog
>>>>
>>>> 2015-02-25  Kai Tietz  <ktietz@redhat.com>
>>>>
>>>>     PR tree-optimization/61917
>>>>     * tree-vect-loop.c (vectorizable_reduction): Allow
>>>>     vect_internal_def without reduction to exit graceful.
>>>>
>>>> ChagneLog testsuite/
>>>>
>>>> 2015-02-25  Kai Tietz  <ktietz@redhat.com>
>>>>
>>>>     PR tree-optimization/61917
>>>>     * gcc.dg/vect/vect-pr61917.c: New file.
>>>>
>>>> Tested for x86_64-unkown-linux.  Ok for apply?
>>>
>>> It doesn't make much sense to fail here as said in the comment
>>> because of patterns if the actual case isn't a pattern.  Also
>>> the patch causing this made vect_external_def possible, so
>>> why does this affect vect_internal_def here?
>>>
>>> It may paper over the issue - but clearly the fix is bogus.
>>
>> Well, actually I don't see any good reason for failing here at all,
>> but I assumed that author wanted to get by it some missed cases.
>> AFAIU the code we actually don't need/want to assert here either on
>> internal (where it is pretty likely no pattern present), and also on
>> externals, too.
>> I would be fine to remove this assert here completely, but I thought
>> it might be of interest to see that orig_stmt isn't NULL for nested
>> case
>
> I agree that the code is overusing asserts but the path you bail out
> on is bogus.  Fact is that we somehow detected a reduction here
> (via special-casing of minus I guess) but fail to properly handle it
> later.  In fact we assert that we end up with a PHI node in the definition
> but would ICE there as well.  Thus a better patch would be
>
> Index: gcc/tree-vect-loop.c
> ===================================================================
> --- gcc/tree-vect-loop.c        (revision 220958)
> +++ gcc/tree-vect-loop.c        (working copy)
> @@ -4981,6 +4981,13 @@ vectorizable_reduction (gimple stmt, gim
>    if (!vectype_in)
>      vectype_in = tem;
>    gcc_assert (is_simple_use);
> +
> +  /* If the defining stmt isn't a PHI node then this isn't a reduction.  */
> +  if (!found_nested_cycle_def)
> +    reduc_def_stmt = def_stmt;
> +  if (gimple_code (reduc_def_stmt) != GIMPLE_PHI)
> +    return false;
> +
>    if (!(dt == vect_reduction_def
>         || dt == vect_nested_cycle
>         || ((dt == vect_internal_def || dt == vect_external_def
> @@ -4993,10 +5000,7 @@ vectorizable_reduction (gimple stmt, gim
>        gcc_assert (orig_stmt);
>        return false;
>      }
> -  if (!found_nested_cycle_def)
> -    reduc_def_stmt = def_stmt;
>
> -  gcc_assert (gimple_code (reduc_def_stmt) == GIMPLE_PHI);
>    if (orig_stmt)
>      gcc_assert (orig_stmt == vect_is_simple_reduction (loop_vinfo,
>                                                         reduc_def_stmt,
>
> Richard.

Yes, your version is better for reader and shows the intention better.
I will give it a test.

Kai

>>> Richard.
>>>
>>>> Regards,
>>>> Kai
>>>>
>>>> Index: tree-vect-loop.c
>>>> ===================================================================
>>>> --- tree-vect-loop.c    (Revision 220958)
>>>> +++ tree-vect-loop.c    (Arbeitskopie)
>>>> @@ -4990,7 +4990,7 @@ vectorizable_reduction (gimple stmt, gimple_stmt_i
>>>>        /* For pattern recognized stmts, orig_stmt might be a reduction,
>>>>       but some helper statements for the pattern might not, or
>>>>       might be COND_EXPRs with reduction uses in the condition.  */
>>>> -      gcc_assert (orig_stmt);
>>>> +      gcc_assert (orig_stmt || dt == vect_internal_def);
>>>>        return false;
>>>>      }
>>>>    if (!found_nested_cycle_def)
>>>> Index: gcc/gcc/testsuite/gcc.dg/vect/vect-pr61917.c
>>>> ===================================================================
>>>> --- /dev/null
>>>> +++ gcc/gcc/testsuite/gcc.dg/vect/vect-pr61917.c
>>>> @@ -0,0 +1,13 @@
>>>> +/* { dg-do compile } */
>>>> +/* { dg-additional-options "-O3" } */
>>>> +
>>>> +int a, b, c, d;
>>>> +
>>>> +int
>>>> +fn1 ()
>>>> +{
>>>> +  for (; c; c++)
>>>> +    for (b = 0; b < 2; b++)
>>>> +      d = a - d;
>>>> +  return d;
>>>> +}

Patch
diff mbox

Index: gcc/tree-vect-loop.c
===================================================================
--- gcc/tree-vect-loop.c        (revision 220958)
+++ gcc/tree-vect-loop.c        (working copy)
@@ -4981,6 +4981,13 @@  vectorizable_reduction (gimple stmt, gim
   if (!vectype_in)
     vectype_in = tem;
   gcc_assert (is_simple_use);
+
+  /* If the defining stmt isn't a PHI node then this isn't a reduction.  */
+  if (!found_nested_cycle_def)
+    reduc_def_stmt = def_stmt;
+  if (gimple_code (reduc_def_stmt) != GIMPLE_PHI)
+    return false;
+
   if (!(dt == vect_reduction_def
        || dt == vect_nested_cycle
        || ((dt == vect_internal_def || dt == vect_external_def
@@ -4993,10 +5000,7 @@  vectorizable_reduction (gimple stmt, gim
       gcc_assert (orig_stmt);
       return false;
     }
-  if (!found_nested_cycle_def)
-    reduc_def_stmt = def_stmt;

-  gcc_assert (gimple_code (reduc_def_stmt) == GIMPLE_PHI);
   if (orig_stmt)
     gcc_assert (orig_stmt == vect_is_simple_reduction (loop_vinfo,
                                                        reduc_def_stmt,