| Message ID | CAFiYyc0abfFKJqzzg_eX=z5FPkduvpwPyK0upLu8LoKef4Hiyg@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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; >>>> +}
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,