On Wed, Feb 25, 2015 at 12:05 PM, Kai Tietz <ktiet...@googlemail.com> wrote:
> 2015-02-25 11:57 GMT+01:00 Richard Biener <richard.guent...@gmail.com>:
>> On Wed, Feb 25, 2015 at 11:06 AM, Kai Tietz <ktiet...@googlemail.com> wrote:
>>> Hello,
>>>
>>> ChangeLog
>>>
>>> 2015-02-25 Kai Tietz <kti...@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 <kti...@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.
> 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;
>>> +}
