| Message ID | 1501155806-5093-1-git-send-email-james.greenhalgh@arm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 27, 2017 at 1:43 PM, James Greenhalgh <james.greenhalgh@arm.com> wrote: > > Hi, > > While answering a user question on the equivalence of > -ftree-loop-vectorize + -ftree-slp-vectorize and -ftree-vectorize I > spotted one case which broke the equivalence. pass_ch::process_loop_p > was guarded on flag_tree_vectorize, meaning you would get it for > -ftree-vectorize, but not for -ftree-loop-vectorize/-ftree-slp-vectorize. > > This patch fixes that, getting rid of the only use of flag_tree_vectorize > in the code base. > > This was preapproved on IRC: > > <jgreenhalgh> binche01: Should the first check in > gcc/tree-ssa-loop-ch.c :: pass_ch_vect::process_loop_p really be on > !flag_tree_vectorize ? That seems to go against the documentation that > -ftree-vectorize is equivalent to -ftree-loop-vectorize > -ftree-slp-vectorize > <binche01> never noticed the condition. any trouble caused? > <jgreenhalgh> None that I know of, I was trying to answer a user > question of whether the flags were really equivalent, and spotted > that while grepping to confirm it > <binche01> don't know if header copy can enables slp with > -fno-tree-loop-vectorize. richi may have the answer. maybe you > can change it to flag_tree_loop_* see if there is breakage. > <richi> jgreenhalgh: we should remove flag_tree_vectorize > <richi> jgreenhalgh: patch pre-approved and change the CH flag check > to flag_tree_loop_vectorize > > Committed as r250619 after a successful bootstrap and test run on > aarch64-none-linux-gnu. > > I'm not sure what was meant by "remove flag_tree_vectorize" - the command line > option seems a bit too popular to deprecate it, and the options framework > doesn't like the idea of one option as an Alias of two others. So I've > left it in place pending further instructions. I thought of Index: gcc/common.opt =================================================================== --- gcc/common.opt (revision 250619) +++ gcc/common.opt (working copy) ftree-vectorize -Common Report Var(flag_tree_vectorize) Optimization +Common Report Optimization Enable vectorization on trees. ftree-vectorizer-verbose= which shows a few other uses of flag_tree_vectorize: int omp_max_vf (void) { if (!optimize || optimize_debug || !flag_tree_loop_optimize || (!flag_tree_loop_vectorize && (global_options_set.x_flag_tree_loop_vectorize || global_options_set.x_flag_tree_vectorize))) return 1; not sure what that was supposed to test. Jakub? Similar use in expand_omp_simd. I suppose it wants to check whether _any_ function has loop vectorization enabled (for some reason)? And this is an approximation only (doesn't work for one function with attribute(optimize("tree-loop-vectorize")). Thanks, Richard. > Thanks, > James > > --- > 2017-07-27 James Greenhalgh <james.greenhalgh@arm.com> > > * tree-ssa-loop-ch.c (pass_ch::process_loop_p): Guard on > flag_tree_loop_vectorize rather than flag_tree_vectorize. >
On Thu, Jul 27, 2017 at 01:54:21PM +0200, Richard Biener wrote: > --- gcc/common.opt (revision 250619) > +++ gcc/common.opt (working copy) > ftree-vectorize > -Common Report Var(flag_tree_vectorize) Optimization > +Common Report Optimization > Enable vectorization on trees. > > ftree-vectorizer-verbose= > > which shows a few other uses of flag_tree_vectorize: > > int > omp_max_vf (void) > { > if (!optimize > || optimize_debug > || !flag_tree_loop_optimize > || (!flag_tree_loop_vectorize > && (global_options_set.x_flag_tree_loop_vectorize > || global_options_set.x_flag_tree_vectorize))) > return 1; > > not sure what that was supposed to test. Jakub? Similar > use in expand_omp_simd. The intent is/was to check if the vectorizer pass will be invoked, otherwise it makes no sense to generate the arrays. So, for -O0/-Og or -fno-tree-loop-optimize, we know that the pass isn't even in the pipeline. And otherwise the intent was that we try to optimize, unless user asked explicitly -fno-tree-loop-vectorize or -fno-tree-vectorize not to optimize. Because the vect pass is enabled if: flag_tree_loop_vectorize || fun->has_force_vectorize_loops but returning non-zero from omp_max_vf and the other omp spot means there will be cfun->has_force_vectorize_loops. Jakub
On Thu, Jul 27, 2017 at 2:08 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Jul 27, 2017 at 01:54:21PM +0200, Richard Biener wrote: >> --- gcc/common.opt (revision 250619) >> +++ gcc/common.opt (working copy) >> ftree-vectorize >> -Common Report Var(flag_tree_vectorize) Optimization >> +Common Report Optimization >> Enable vectorization on trees. >> >> ftree-vectorizer-verbose= >> >> which shows a few other uses of flag_tree_vectorize: >> >> int >> omp_max_vf (void) >> { >> if (!optimize >> || optimize_debug >> || !flag_tree_loop_optimize >> || (!flag_tree_loop_vectorize >> && (global_options_set.x_flag_tree_loop_vectorize >> || global_options_set.x_flag_tree_vectorize))) >> return 1; >> >> not sure what that was supposed to test. Jakub? Similar >> use in expand_omp_simd. > > The intent is/was to check if the vectorizer pass will be invoked, > otherwise it makes no sense to generate the arrays. > So, for -O0/-Og or -fno-tree-loop-optimize, we know that the pass > isn't even in the pipeline. > And otherwise the intent was that we try to optimize, unless > user asked explicitly -fno-tree-loop-vectorize or -fno-tree-vectorize > not to optimize. Because the vect pass is enabled if: > flag_tree_loop_vectorize || fun->has_force_vectorize_loops > but returning non-zero from omp_max_vf and the other omp spot means > there will be cfun->has_force_vectorize_loops. I see. So it would be good to try if adding EnabledBy(ftree-vectorize) to ftree-loop-vectorize/ftree-slp-vectorize would add those to global_options_set iff -ftree-vectorize is enabled (the opts.c hunk setting the flags is then redundant as well I guess). Richard. > Jakub
On Thu, Jul 27, 2017 at 02:26:03PM +0200, Richard Biener wrote: > On Thu, Jul 27, 2017 at 2:08 PM, Jakub Jelinek <jakub@redhat.com> wrote: > > On Thu, Jul 27, 2017 at 01:54:21PM +0200, Richard Biener wrote: > >> --- gcc/common.opt (revision 250619) > >> +++ gcc/common.opt (working copy) > >> ftree-vectorize > >> -Common Report Var(flag_tree_vectorize) Optimization > >> +Common Report Optimization > >> Enable vectorization on trees. > >> > >> ftree-vectorizer-verbose= > >> > >> which shows a few other uses of flag_tree_vectorize: > >> > >> int > >> omp_max_vf (void) > >> { > >> if (!optimize > >> || optimize_debug > >> || !flag_tree_loop_optimize > >> || (!flag_tree_loop_vectorize > >> && (global_options_set.x_flag_tree_loop_vectorize > >> || global_options_set.x_flag_tree_vectorize))) > >> return 1; > >> > >> not sure what that was supposed to test. Jakub? Similar > >> use in expand_omp_simd. > > > > The intent is/was to check if the vectorizer pass will be invoked, > > otherwise it makes no sense to generate the arrays. > > So, for -O0/-Og or -fno-tree-loop-optimize, we know that the pass > > isn't even in the pipeline. > > And otherwise the intent was that we try to optimize, unless > > user asked explicitly -fno-tree-loop-vectorize or -fno-tree-vectorize > > not to optimize. Because the vect pass is enabled if: > > flag_tree_loop_vectorize || fun->has_force_vectorize_loops > > but returning non-zero from omp_max_vf and the other omp spot means > > there will be cfun->has_force_vectorize_loops. > > I see. So it would be good to try if adding EnabledBy(ftree-vectorize) to > ftree-loop-vectorize/ftree-slp-vectorize would add those to global_options_set > iff -ftree-vectorize is enabled (the opts.c hunk setting the flags is then > redundant as well I guess). This looks like it works. I'll prepare the patch and put it through a full bootstrap cycle. Thanks, James
diff --git a/gcc/tree-ssa-loop-ch.c b/gcc/tree-ssa-loop-ch.c index 86be34a..14cc6d8d 100644 --- a/gcc/tree-ssa-loop-ch.c +++ b/gcc/tree-ssa-loop-ch.c @@ -436,7 +436,7 @@ pass_ch::process_loop_p (struct loop *loop) bool pass_ch_vect::process_loop_p (struct loop *loop) { - if (!flag_tree_vectorize && !loop->force_vectorize) + if (!flag_tree_loop_vectorize && !loop->force_vectorize) return false; if (loop->dont_vectorize)
Hi, While answering a user question on the equivalence of -ftree-loop-vectorize + -ftree-slp-vectorize and -ftree-vectorize I spotted one case which broke the equivalence. pass_ch::process_loop_p was guarded on flag_tree_vectorize, meaning you would get it for -ftree-vectorize, but not for -ftree-loop-vectorize/-ftree-slp-vectorize. This patch fixes that, getting rid of the only use of flag_tree_vectorize in the code base. This was preapproved on IRC: <jgreenhalgh> binche01: Should the first check in gcc/tree-ssa-loop-ch.c :: pass_ch_vect::process_loop_p really be on !flag_tree_vectorize ? That seems to go against the documentation that -ftree-vectorize is equivalent to -ftree-loop-vectorize -ftree-slp-vectorize <binche01> never noticed the condition. any trouble caused? <jgreenhalgh> None that I know of, I was trying to answer a user question of whether the flags were really equivalent, and spotted that while grepping to confirm it <binche01> don't know if header copy can enables slp with -fno-tree-loop-vectorize. richi may have the answer. maybe you can change it to flag_tree_loop_* see if there is breakage. <richi> jgreenhalgh: we should remove flag_tree_vectorize <richi> jgreenhalgh: patch pre-approved and change the CH flag check to flag_tree_loop_vectorize Committed as r250619 after a successful bootstrap and test run on aarch64-none-linux-gnu. I'm not sure what was meant by "remove flag_tree_vectorize" - the command line option seems a bit too popular to deprecate it, and the options framework doesn't like the idea of one option as an Alias of two others. So I've left it in place pending further instructions. Thanks, James --- 2017-07-27 James Greenhalgh <james.greenhalgh@arm.com> * tree-ssa-loop-ch.c (pass_ch::process_loop_p): Guard on flag_tree_loop_vectorize rather than flag_tree_vectorize.