| Message ID | 87606rsivf.fsf@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make fix for PR 83965 handle SLP reduction chains | expand |
On Tue, Feb 20, 2018 at 5:53 PM, Richard Sandiford <richard.sandiford@linaro.org> wrote: > This patch prevents pattern-matching of fold-left SLP reduction chains, > which the previous patch for 83965 didn't handle properly. It only > stops the last statement in the group from being matched, but that's > enough to cause the group to be dissolved later. > > A better fix would be to put all the information about the reduction > on the the first statement in the reduction chain, so that every > statement in the group can tell what the group is doing. That doesn't > seem like stage 4 material though. > > As it stands, things seem to be a bit of a mess. In > vect_force_simple_reduction we attach the reduction type and > phi pointer to the last statement in a reduction chain: > > reduc_def_info = vinfo_for_stmt (def); > STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type; > STMT_VINFO_REDUC_DEF (reduc_def_info) = phi; > > and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1: > > STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) = > vect_reduction_def; > > This code in vectorizable_reduction gave the impression that > everything really is keyed off the last statement: > > /* In case of reduction chain we switch to the first stmt in the chain, but > we don't update STMT_INFO, since only the last stmt is marked as reduction > and has reduction properties. */ > if (GROUP_FIRST_ELEMENT (stmt_info) > && GROUP_FIRST_ELEMENT (stmt_info) != stmt) > { > stmt = GROUP_FIRST_ELEMENT (stmt_info); > first_p = false; > } > > But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull > for SLP reduction chains, since we dissolve the group if SLP fails. > And SLP only analyses the first statement in the group, not the last: > > stmt = SLP_TREE_SCALAR_STMTS (node)[0]; > stmt_vec_info stmt_info = vinfo_for_stmt (stmt); > [...] > bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance); > > So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF > are being attached to the wrong statement, since we only analyse > the first one. But it turns out that REDUC_TYPE and REDUC_DEF > don't matter for the first statement in the group, since that > takes the phi as an input, and when the phi is a direct input, > we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's > own info. The DEF_TYPE problem is handled by: > > /* Mark the first element of the reduction chain as reduction to properly > transform the node. In the reduction analysis phase only the last > element of the chain is marked as reduction. */ > if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt))) > STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def; > > in vect_analyze_slp_instance (cancelled by: > > STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element)) > = vect_internal_def; > > in vect_analyze_slp on failure), with the operation being repeated > in vect_schedule_slp_instance (redundantly AFAICT): > > /* Mark the first element of the reduction chain as reduction to properly > transform the node. In the analysis phase only the last element of the > chain is marked as reduction. */ > if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS (stmt_info) > && GROUP_FIRST_ELEMENT (stmt_info) == stmt) > { > STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def; > STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type; > } > > Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu. > OK to install? Ok for stage1. Richard. > Richard > > > 2018-02-20 Richard Sandiford <richard.sandiford@linaro.org> > > gcc/ > PR tree-optimization/83965 > * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume > that grouped statements are part of a reduction chain. Return > true if the statement is not marked as a reduction itself but > is part of a group. > (vect_recog_dot_prod_pattern): Don't check whether the statement > is part of a group here. > (vect_recog_sad_pattern): Likewise. > (vect_recog_widen_sum_pattern): Likewise. > > gcc/testsuite/ > PR tree-optimization/83965 > * gcc.dg/vect/pr83965-2.c: New test. > > Index: gcc/tree-vect-patterns.c > =================================================================== > --- gcc/tree-vect-patterns.c 2018-02-20 09:40:41.843451227 +0000 > +++ gcc/tree-vect-patterns.c 2018-02-20 16:28:55.636762056 +0000 > @@ -222,13 +222,16 @@ vect_recog_temp_ssa_var (tree type, gimp > } > > /* Return true if STMT_VINFO describes a reduction for which reassociation > - is allowed. */ > + is allowed. If STMT_INFO is part of a group, assume that it's part of > + a reduction chain and optimistically assume that all statements > + except the last allow reassociation. */ > > static bool > vect_reassociating_reduction_p (stmt_vec_info stmt_vinfo) > { > return (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def > - && STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION); > + ? STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION > + : GROUP_FIRST_ELEMENT (stmt_vinfo) != NULL); > } > > /* Function vect_recog_dot_prod_pattern > @@ -350,8 +353,7 @@ vect_recog_dot_prod_pattern (vec<gimple > { > gimple *def_stmt; > > - if (!vect_reassociating_reduction_p (stmt_vinfo) > - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) > + if (!vect_reassociating_reduction_p (stmt_vinfo)) > return NULL; > oprnd0 = gimple_assign_rhs1 (last_stmt); > oprnd1 = gimple_assign_rhs2 (last_stmt); > @@ -571,8 +573,7 @@ vect_recog_sad_pattern (vec<gimple *> *s > { > gimple *def_stmt; > > - if (!vect_reassociating_reduction_p (stmt_vinfo) > - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) > + if (!vect_reassociating_reduction_p (stmt_vinfo)) > return NULL; > plus_oprnd0 = gimple_assign_rhs1 (last_stmt); > plus_oprnd1 = gimple_assign_rhs2 (last_stmt); > @@ -1256,8 +1257,7 @@ vect_recog_widen_sum_pattern (vec<gimple > if (gimple_assign_rhs_code (last_stmt) != PLUS_EXPR) > return NULL; > > - if (!vect_reassociating_reduction_p (stmt_vinfo) > - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) > + if (!vect_reassociating_reduction_p (stmt_vinfo)) > return NULL; > > oprnd0 = gimple_assign_rhs1 (last_stmt); > Index: gcc/testsuite/gcc.dg/vect/pr83965-2.c > =================================================================== > --- /dev/null 2018-02-19 19:34:42.906488063 +0000 > +++ gcc/testsuite/gcc.dg/vect/pr83965-2.c 2018-02-20 16:28:55.635762095 +0000 > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-Ofast -ftrapv" } */ > + > +int c; > +unsigned char d; > +int e (unsigned char *f) > +{ > + int g; > + for (int a; a; a++) > + { > + for (int b = 0; b < 6; b++) > + g += __builtin_abs (f[b] - d); > + f += c; > + } > + return g; > +}
Richard Biener <richard.guenther@gmail.com> writes: > On Tue, Feb 20, 2018 at 5:53 PM, Richard Sandiford > <richard.sandiford@linaro.org> wrote: >> This patch prevents pattern-matching of fold-left SLP reduction chains, >> which the previous patch for 83965 didn't handle properly. It only >> stops the last statement in the group from being matched, but that's >> enough to cause the group to be dissolved later. >> >> A better fix would be to put all the information about the reduction >> on the the first statement in the reduction chain, so that every >> statement in the group can tell what the group is doing. That doesn't >> seem like stage 4 material though. >> >> As it stands, things seem to be a bit of a mess. In >> vect_force_simple_reduction we attach the reduction type and >> phi pointer to the last statement in a reduction chain: >> >> reduc_def_info = vinfo_for_stmt (def); >> STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type; >> STMT_VINFO_REDUC_DEF (reduc_def_info) = phi; >> >> and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1: >> >> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) = >> vect_reduction_def; >> >> This code in vectorizable_reduction gave the impression that >> everything really is keyed off the last statement: >> >> /* In case of reduction chain we switch to the first stmt in the chain, but >> we don't update STMT_INFO, since only the last stmt is marked as reduction >> and has reduction properties. */ >> if (GROUP_FIRST_ELEMENT (stmt_info) >> && GROUP_FIRST_ELEMENT (stmt_info) != stmt) >> { >> stmt = GROUP_FIRST_ELEMENT (stmt_info); >> first_p = false; >> } >> >> But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull >> for SLP reduction chains, since we dissolve the group if SLP fails. >> And SLP only analyses the first statement in the group, not the last: >> >> stmt = SLP_TREE_SCALAR_STMTS (node)[0]; >> stmt_vec_info stmt_info = vinfo_for_stmt (stmt); >> [...] >> bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance); >> >> So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF >> are being attached to the wrong statement, since we only analyse >> the first one. But it turns out that REDUC_TYPE and REDUC_DEF >> don't matter for the first statement in the group, since that >> takes the phi as an input, and when the phi is a direct input, >> we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's >> own info. The DEF_TYPE problem is handled by: >> >> /* Mark the first element of the reduction chain as reduction to properly >> transform the node. In the reduction analysis phase only the last >> element of the chain is marked as reduction. */ >> if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt))) >> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def; >> >> in vect_analyze_slp_instance (cancelled by: >> >> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element)) >> = vect_internal_def; >> >> in vect_analyze_slp on failure), with the operation being repeated >> in vect_schedule_slp_instance (redundantly AFAICT): >> >> /* Mark the first element of the reduction chain as reduction to properly >> transform the node. In the analysis phase only the last element of the >> chain is marked as reduction. */ >> if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS (stmt_info) >> && GROUP_FIRST_ELEMENT (stmt_info) == stmt) >> { >> STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def; >> STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type; >> } >> >> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu. >> OK to install? > > Ok for stage1. It's a GCC 8 regression, so OK for stage4? Richard > Richard. > >> Richard >> >> >> 2018-02-20 Richard Sandiford <richard.sandiford@linaro.org> >> >> gcc/ >> PR tree-optimization/83965 >> * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume >> that grouped statements are part of a reduction chain. Return >> true if the statement is not marked as a reduction itself but >> is part of a group. >> (vect_recog_dot_prod_pattern): Don't check whether the statement >> is part of a group here. >> (vect_recog_sad_pattern): Likewise. >> (vect_recog_widen_sum_pattern): Likewise. >> >> gcc/testsuite/ >> PR tree-optimization/83965 >> * gcc.dg/vect/pr83965-2.c: New test. >> >> Index: gcc/tree-vect-patterns.c >> =================================================================== >> --- gcc/tree-vect-patterns.c 2018-02-20 09:40:41.843451227 +0000 >> +++ gcc/tree-vect-patterns.c 2018-02-20 16:28:55.636762056 +0000 >> @@ -222,13 +222,16 @@ vect_recog_temp_ssa_var (tree type, gimp >> } >> >> /* Return true if STMT_VINFO describes a reduction for which reassociation >> - is allowed. */ >> + is allowed. If STMT_INFO is part of a group, assume that it's part of >> + a reduction chain and optimistically assume that all statements >> + except the last allow reassociation. */ >> >> static bool >> vect_reassociating_reduction_p (stmt_vec_info stmt_vinfo) >> { >> return (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def >> - && STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION); >> + ? STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION >> + : GROUP_FIRST_ELEMENT (stmt_vinfo) != NULL); >> } >> >> /* Function vect_recog_dot_prod_pattern >> @@ -350,8 +353,7 @@ vect_recog_dot_prod_pattern (vec<gimple >> { >> gimple *def_stmt; >> >> - if (!vect_reassociating_reduction_p (stmt_vinfo) >> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >> return NULL; >> oprnd0 = gimple_assign_rhs1 (last_stmt); >> oprnd1 = gimple_assign_rhs2 (last_stmt); >> @@ -571,8 +573,7 @@ vect_recog_sad_pattern (vec<gimple *> *s >> { >> gimple *def_stmt; >> >> - if (!vect_reassociating_reduction_p (stmt_vinfo) >> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >> return NULL; >> plus_oprnd0 = gimple_assign_rhs1 (last_stmt); >> plus_oprnd1 = gimple_assign_rhs2 (last_stmt); >> @@ -1256,8 +1257,7 @@ vect_recog_widen_sum_pattern (vec<gimple >> if (gimple_assign_rhs_code (last_stmt) != PLUS_EXPR) >> return NULL; >> >> - if (!vect_reassociating_reduction_p (stmt_vinfo) >> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >> return NULL; >> >> oprnd0 = gimple_assign_rhs1 (last_stmt); >> Index: gcc/testsuite/gcc.dg/vect/pr83965-2.c >> =================================================================== >> --- /dev/null 2018-02-19 19:34:42.906488063 +0000 >> +++ gcc/testsuite/gcc.dg/vect/pr83965-2.c 2018-02-20 16:28:55.635762095 +0000 >> @@ -0,0 +1,16 @@ >> +/* { dg-do compile } */ >> +/* { dg-additional-options "-Ofast -ftrapv" } */ >> + >> +int c; >> +unsigned char d; >> +int e (unsigned char *f) >> +{ >> + int g; >> + for (int a; a; a++) >> + { >> + for (int b = 0; b < 6; b++) >> + g += __builtin_abs (f[b] - d); >> + f += c; >> + } >> + return g; >> +}
On Mon, Feb 26, 2018 at 3:37 PM, Richard Sandiford <richard.sandiford@linaro.org> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Tue, Feb 20, 2018 at 5:53 PM, Richard Sandiford >> <richard.sandiford@linaro.org> wrote: >>> This patch prevents pattern-matching of fold-left SLP reduction chains, >>> which the previous patch for 83965 didn't handle properly. It only >>> stops the last statement in the group from being matched, but that's >>> enough to cause the group to be dissolved later. >>> >>> A better fix would be to put all the information about the reduction >>> on the the first statement in the reduction chain, so that every >>> statement in the group can tell what the group is doing. That doesn't >>> seem like stage 4 material though. >>> >>> As it stands, things seem to be a bit of a mess. In >>> vect_force_simple_reduction we attach the reduction type and >>> phi pointer to the last statement in a reduction chain: >>> >>> reduc_def_info = vinfo_for_stmt (def); >>> STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type; >>> STMT_VINFO_REDUC_DEF (reduc_def_info) = phi; >>> >>> and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1: >>> >>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) = >>> vect_reduction_def; >>> >>> This code in vectorizable_reduction gave the impression that >>> everything really is keyed off the last statement: >>> >>> /* In case of reduction chain we switch to the first stmt in the chain, but >>> we don't update STMT_INFO, since only the last stmt is marked as reduction >>> and has reduction properties. */ >>> if (GROUP_FIRST_ELEMENT (stmt_info) >>> && GROUP_FIRST_ELEMENT (stmt_info) != stmt) >>> { >>> stmt = GROUP_FIRST_ELEMENT (stmt_info); >>> first_p = false; >>> } >>> >>> But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull >>> for SLP reduction chains, since we dissolve the group if SLP fails. >>> And SLP only analyses the first statement in the group, not the last: >>> >>> stmt = SLP_TREE_SCALAR_STMTS (node)[0]; >>> stmt_vec_info stmt_info = vinfo_for_stmt (stmt); >>> [...] >>> bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance); >>> >>> So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF >>> are being attached to the wrong statement, since we only analyse >>> the first one. But it turns out that REDUC_TYPE and REDUC_DEF >>> don't matter for the first statement in the group, since that >>> takes the phi as an input, and when the phi is a direct input, >>> we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's >>> own info. The DEF_TYPE problem is handled by: >>> >>> /* Mark the first element of the reduction chain as reduction to properly >>> transform the node. In the reduction analysis phase only the last >>> element of the chain is marked as reduction. */ >>> if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt))) >>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def; >>> >>> in vect_analyze_slp_instance (cancelled by: >>> >>> STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element)) >>> = vect_internal_def; >>> >>> in vect_analyze_slp on failure), with the operation being repeated >>> in vect_schedule_slp_instance (redundantly AFAICT): >>> >>> /* Mark the first element of the reduction chain as reduction to properly >>> transform the node. In the analysis phase only the last element of the >>> chain is marked as reduction. */ >>> if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS (stmt_info) >>> && GROUP_FIRST_ELEMENT (stmt_info) == stmt) >>> { >>> STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def; >>> STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type; >>> } >>> >>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu. >>> OK to install? >> >> Ok for stage1. > > It's a GCC 8 regression, so OK for stage4? Oh, ok then. Richard. > Richard > >> Richard. >> >>> Richard >>> >>> >>> 2018-02-20 Richard Sandiford <richard.sandiford@linaro.org> >>> >>> gcc/ >>> PR tree-optimization/83965 >>> * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume >>> that grouped statements are part of a reduction chain. Return >>> true if the statement is not marked as a reduction itself but >>> is part of a group. >>> (vect_recog_dot_prod_pattern): Don't check whether the statement >>> is part of a group here. >>> (vect_recog_sad_pattern): Likewise. >>> (vect_recog_widen_sum_pattern): Likewise. >>> >>> gcc/testsuite/ >>> PR tree-optimization/83965 >>> * gcc.dg/vect/pr83965-2.c: New test. >>> >>> Index: gcc/tree-vect-patterns.c >>> =================================================================== >>> --- gcc/tree-vect-patterns.c 2018-02-20 09:40:41.843451227 +0000 >>> +++ gcc/tree-vect-patterns.c 2018-02-20 16:28:55.636762056 +0000 >>> @@ -222,13 +222,16 @@ vect_recog_temp_ssa_var (tree type, gimp >>> } >>> >>> /* Return true if STMT_VINFO describes a reduction for which reassociation >>> - is allowed. */ >>> + is allowed. If STMT_INFO is part of a group, assume that it's part of >>> + a reduction chain and optimistically assume that all statements >>> + except the last allow reassociation. */ >>> >>> static bool >>> vect_reassociating_reduction_p (stmt_vec_info stmt_vinfo) >>> { >>> return (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def >>> - && STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION); >>> + ? STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION >>> + : GROUP_FIRST_ELEMENT (stmt_vinfo) != NULL); >>> } >>> >>> /* Function vect_recog_dot_prod_pattern >>> @@ -350,8 +353,7 @@ vect_recog_dot_prod_pattern (vec<gimple >>> { >>> gimple *def_stmt; >>> >>> - if (!vect_reassociating_reduction_p (stmt_vinfo) >>> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >>> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >>> return NULL; >>> oprnd0 = gimple_assign_rhs1 (last_stmt); >>> oprnd1 = gimple_assign_rhs2 (last_stmt); >>> @@ -571,8 +573,7 @@ vect_recog_sad_pattern (vec<gimple *> *s >>> { >>> gimple *def_stmt; >>> >>> - if (!vect_reassociating_reduction_p (stmt_vinfo) >>> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >>> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >>> return NULL; >>> plus_oprnd0 = gimple_assign_rhs1 (last_stmt); >>> plus_oprnd1 = gimple_assign_rhs2 (last_stmt); >>> @@ -1256,8 +1257,7 @@ vect_recog_widen_sum_pattern (vec<gimple >>> if (gimple_assign_rhs_code (last_stmt) != PLUS_EXPR) >>> return NULL; >>> >>> - if (!vect_reassociating_reduction_p (stmt_vinfo) >>> - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) >>> + if (!vect_reassociating_reduction_p (stmt_vinfo)) >>> return NULL; >>> >>> oprnd0 = gimple_assign_rhs1 (last_stmt); >>> Index: gcc/testsuite/gcc.dg/vect/pr83965-2.c >>> =================================================================== >>> --- /dev/null 2018-02-19 19:34:42.906488063 +0000 >>> +++ gcc/testsuite/gcc.dg/vect/pr83965-2.c 2018-02-20 16:28:55.635762095 +0000 >>> @@ -0,0 +1,16 @@ >>> +/* { dg-do compile } */ >>> +/* { dg-additional-options "-Ofast -ftrapv" } */ >>> + >>> +int c; >>> +unsigned char d; >>> +int e (unsigned char *f) >>> +{ >>> + int g; >>> + for (int a; a; a++) >>> + { >>> + for (int b = 0; b < 6; b++) >>> + g += __builtin_abs (f[b] - d); >>> + f += c; >>> + } >>> + return g; >>> +}
Index: gcc/tree-vect-patterns.c =================================================================== --- gcc/tree-vect-patterns.c 2018-02-20 09:40:41.843451227 +0000 +++ gcc/tree-vect-patterns.c 2018-02-20 16:28:55.636762056 +0000 @@ -222,13 +222,16 @@ vect_recog_temp_ssa_var (tree type, gimp } /* Return true if STMT_VINFO describes a reduction for which reassociation - is allowed. */ + is allowed. If STMT_INFO is part of a group, assume that it's part of + a reduction chain and optimistically assume that all statements + except the last allow reassociation. */ static bool vect_reassociating_reduction_p (stmt_vec_info stmt_vinfo) { return (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def - && STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION); + ? STMT_VINFO_REDUC_TYPE (stmt_vinfo) != FOLD_LEFT_REDUCTION + : GROUP_FIRST_ELEMENT (stmt_vinfo) != NULL); } /* Function vect_recog_dot_prod_pattern @@ -350,8 +353,7 @@ vect_recog_dot_prod_pattern (vec<gimple { gimple *def_stmt; - if (!vect_reassociating_reduction_p (stmt_vinfo) - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) + if (!vect_reassociating_reduction_p (stmt_vinfo)) return NULL; oprnd0 = gimple_assign_rhs1 (last_stmt); oprnd1 = gimple_assign_rhs2 (last_stmt); @@ -571,8 +573,7 @@ vect_recog_sad_pattern (vec<gimple *> *s { gimple *def_stmt; - if (!vect_reassociating_reduction_p (stmt_vinfo) - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) + if (!vect_reassociating_reduction_p (stmt_vinfo)) return NULL; plus_oprnd0 = gimple_assign_rhs1 (last_stmt); plus_oprnd1 = gimple_assign_rhs2 (last_stmt); @@ -1256,8 +1257,7 @@ vect_recog_widen_sum_pattern (vec<gimple if (gimple_assign_rhs_code (last_stmt) != PLUS_EXPR) return NULL; - if (!vect_reassociating_reduction_p (stmt_vinfo) - && ! STMT_VINFO_GROUP_FIRST_ELEMENT (stmt_vinfo)) + if (!vect_reassociating_reduction_p (stmt_vinfo)) return NULL; oprnd0 = gimple_assign_rhs1 (last_stmt); Index: gcc/testsuite/gcc.dg/vect/pr83965-2.c =================================================================== --- /dev/null 2018-02-19 19:34:42.906488063 +0000 +++ gcc/testsuite/gcc.dg/vect/pr83965-2.c 2018-02-20 16:28:55.635762095 +0000 @@ -0,0 +1,16 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-Ofast -ftrapv" } */ + +int c; +unsigned char d; +int e (unsigned char *f) +{ + int g; + for (int a; a; a++) + { + for (int b = 0; b < 6; b++) + g += __builtin_abs (f[b] - d); + f += c; + } + return g; +}
This patch prevents pattern-matching of fold-left SLP reduction chains, which the previous patch for 83965 didn't handle properly. It only stops the last statement in the group from being matched, but that's enough to cause the group to be dissolved later. A better fix would be to put all the information about the reduction on the the first statement in the reduction chain, so that every statement in the group can tell what the group is doing. That doesn't seem like stage 4 material though. As it stands, things seem to be a bit of a mess. In vect_force_simple_reduction we attach the reduction type and phi pointer to the last statement in a reduction chain: reduc_def_info = vinfo_for_stmt (def); STMT_VINFO_REDUC_TYPE (reduc_def_info) = v_reduc_type; STMT_VINFO_REDUC_DEF (reduc_def_info) = phi; and mark it as vect_reduction_type in vect_analyze_scalar_cycles_1: STMT_VINFO_DEF_TYPE (vinfo_for_stmt (reduc_stmt)) = vect_reduction_def; This code in vectorizable_reduction gave the impression that everything really is keyed off the last statement: /* In case of reduction chain we switch to the first stmt in the chain, but we don't update STMT_INFO, since only the last stmt is marked as reduction and has reduction properties. */ if (GROUP_FIRST_ELEMENT (stmt_info) && GROUP_FIRST_ELEMENT (stmt_info) != stmt) { stmt = GROUP_FIRST_ELEMENT (stmt_info); first_p = false; } But this code is dead these days. GROUP_FIRST_ELEMENT is only nonnull for SLP reduction chains, since we dissolve the group if SLP fails. And SLP only analyses the first statement in the group, not the last: stmt = SLP_TREE_SCALAR_STMTS (node)[0]; stmt_vec_info stmt_info = vinfo_for_stmt (stmt); [...] bool res = vect_analyze_stmt (stmt, &dummy, node, node_instance); So from that point of view the DEF_TYPE, REDUC_TYPE and REDUC_DEF are being attached to the wrong statement, since we only analyse the first one. But it turns out that REDUC_TYPE and REDUC_DEF don't matter for the first statement in the group, since that takes the phi as an input, and when the phi is a direct input, we use *its* REDUC_TYPE and REDUC_DEF instead of the statement's own info. The DEF_TYPE problem is handled by: /* Mark the first element of the reduction chain as reduction to properly transform the node. In the reduction analysis phase only the last element of the chain is marked as reduction. */ if (!STMT_VINFO_GROUPED_ACCESS (vinfo_for_stmt (stmt))) STMT_VINFO_DEF_TYPE (vinfo_for_stmt (stmt)) = vect_reduction_def; in vect_analyze_slp_instance (cancelled by: STMT_VINFO_DEF_TYPE (vinfo_for_stmt (first_element)) = vect_internal_def; in vect_analyze_slp on failure), with the operation being repeated in vect_schedule_slp_instance (redundantly AFAICT): /* Mark the first element of the reduction chain as reduction to properly transform the node. In the analysis phase only the last element of the chain is marked as reduction. */ if (GROUP_FIRST_ELEMENT (stmt_info) && !STMT_VINFO_GROUPED_ACCESS (stmt_info) && GROUP_FIRST_ELEMENT (stmt_info) == stmt) { STMT_VINFO_DEF_TYPE (stmt_info) = vect_reduction_def; STMT_VINFO_TYPE (stmt_info) = reduc_vec_info_type; } Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu. OK to install? Richard 2018-02-20 Richard Sandiford <richard.sandiford@linaro.org> gcc/ PR tree-optimization/83965 * tree-vect-patterns.c (vect_reassociating_reduction_p): Assume that grouped statements are part of a reduction chain. Return true if the statement is not marked as a reduction itself but is part of a group. (vect_recog_dot_prod_pattern): Don't check whether the statement is part of a group here. (vect_recog_sad_pattern): Likewise. (vect_recog_widen_sum_pattern): Likewise. gcc/testsuite/ PR tree-optimization/83965 * gcc.dg/vect/pr83965-2.c: New test.