Re: move increase_alignment from simple to regular ipa pass

[Prathamesh Kulkarni <prathamesh dot kulkarni at linaro dot org>]

On 10 June 2016 at 16:47, Richard Biener <rguenther@suse.de> wrote:
> On Fri, 10 Jun 2016, Prathamesh Kulkarni wrote:
>
>> On 10 June 2016 at 01:53, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> On 8 June 2016 at 20:38, Jan Hubicka <hubicka@ucw.cz> wrote:
>> >> >> I think it would be nice to work towards transitioning
>> >> >> flag_section_anchors to a flag on varpool nodes, thereby removing
>> >> >> the Optimization flag from common.opt:fsection-anchors
>> >> >>
>> >> >> That would simplify the walk over varpool candidates.
>> >> >
>> >> > Makes sense to me, too. There are more candidates for sutff that should be
>> >> > variable specific in common.opt (such as variable alignment, -fdata-sctions,
>> >> > -fmerge-constants) and targets.  We may try to do it in an easy to extend way
>> >> > so incrementally we can get rid of those global flags, too.
>> >> In this version I removed Optimization from fsection-anchors entry in
>> >> common.opt,
>> >> and gated the increase_alignment pass on flag_section_anchors != 0.
>> >> Cross tested on arm*-*-*, aarch64*-*-*.
>> >> Does it look OK ?
>> >
>> > If you go this way you will need to do something sane for LTO.  Here one can compile
>> > some object files with -fsection-anchors and other without and link with random setting
>> > (because in traditional compilation linktime flags does not matter).
>> >
>> > For global flags we have magic in merge_and_complain that determines flags to pass
>> > to the LTO compiler.
>> > It is not very robust though.
>> >> >
>> >> > One thing that needs to be done for LTO is sane merging, I guess in this case
>> >> > it is clear that the variable should be anchored when its previaling definition
>> >> > is.
>> >> Um could we determine during WPA if symbol is a section anchor for merging ?
>> >> Seems to me SYMBOL_REF_ANCHOR_P is defined only on DECL_RTL and not at
>> >> tree level.
>> >> Do we have DECL_RTL info available during WPA ?
>> >
>> > We don't have anchros computed, but we can decide whether we want to potentially
>> > anchor the variable if we can.
>> >
>> > I would say all you need is to have section_anchor flag in varpool node itself
>> > which controls RTL production. At varpool_finalize_decl you will set it
>> > according to flag_varpool and stream it to LTO objects. At WPA when doing
>> > linking, the section_anchor flag of the previaling decl wins..
>> Thanks for the suggestions.
>> IIUC, we want to add new section_anchor flag to varpool_node class
>> and set it in varpool_node::finalize_decl and stream it to LTO byte-code,
>> and then during WPA set section_anchor_flag during symbol merging if it is set
>> for prevailing decl.
>
> Yes.
>
>> In the increase_alignment_pass if a vnode has section_anchor flag set,
>> we will walk all functions that reference it to check if they have
>> -ftree-loop-vectorize set.
>> Is that correct ?
>
> Yes.
>
>> Could you please elaborate a bit more on "at varpool_finalize_decl you will
>> set section_anchor flag according to flag_varpool" ?
>> flag_varpool doesn't appear to be defined.
>
> flag_section_anchors.
Hi,
I have done the changes in this version
In varpool_node::finalize_decl,
I just set vnode->section_anchor = flag_section_anchors.
Should that be sufficient ?

I tried with a couple of test-cases, once with prevailing->section_anchors == 1
and once with entry->section_anchors == 1 and it appears
prevailing->section_anchor
always took precedence.
So I wonder if the change to lto_symtab_merge () in the patch is necessary ?

Re-introduced flag_ipa_increase_alignment to gate the pass on, so it runs only
for targets supporting section anchors.
Cross tested  on aarch64*-*-*, arm*-*-*.

Thanks,
Prathamesh
>
> Richard.
>
>> Thanks,
>> Prathamesh
>> >
>> > Honza
>> >>
>> >> Thanks,
>> >> Prathamesh
>> >> >
>> >> > Honza
>> >> >>
>> >> >> Richard.
>> >> >>
>> >> >> > Thanks,
>> >> >> > Prathamesh
>> >> >> > >
>> >> >> > > Honza
>> >> >> > >>
>> >> >> > >> Richard.
>> >> >> >
>> >> >> >
>> >> >>
>> >> >> --
>> >> >> Richard Biener <rguenther@suse.de>
>> >> >> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
>> >
>> >> diff --git a/gcc/common.opt b/gcc/common.opt
>> >> index f0d7196..f93f26c 100644
>> >> --- a/gcc/common.opt
>> >> +++ b/gcc/common.opt
>> >> @@ -2133,7 +2133,7 @@ Common Report Var(flag_sched_dep_count_heuristic) Init(1) Optimization
>> >>  Enable the dependent count heuristic in the scheduler.
>> >>
>> >>  fsection-anchors
>> >> -Common Report Var(flag_section_anchors) Optimization
>> >> +Common Report Var(flag_section_anchors)
>> >>  Access data in the same section from shared anchor points.
>> >>
>> >>  fsee
>> >> diff --git a/gcc/passes.def b/gcc/passes.def
>> >> index 3647e90..3a8063c 100644
>> >> --- a/gcc/passes.def
>> >> +++ b/gcc/passes.def
>> >> @@ -138,12 +138,12 @@ along with GCC; see the file COPYING3.  If not see
>> >>    PUSH_INSERT_PASSES_WITHIN (pass_ipa_tree_profile)
>> >>        NEXT_PASS (pass_feedback_split_functions);
>> >>    POP_INSERT_PASSES ()
>> >> -  NEXT_PASS (pass_ipa_increase_alignment);
>> >>    NEXT_PASS (pass_ipa_tm);
>> >>    NEXT_PASS (pass_ipa_lower_emutls);
>> >>    TERMINATE_PASS_LIST (all_small_ipa_passes)
>> >>
>> >>    INSERT_PASSES_AFTER (all_regular_ipa_passes)
>> >> +  NEXT_PASS (pass_ipa_increase_alignment);
>> >>    NEXT_PASS (pass_ipa_whole_program_visibility);
>> >>    NEXT_PASS (pass_ipa_profile);
>> >>    NEXT_PASS (pass_ipa_icf);
>> >> diff --git a/gcc/testsuite/gcc.dg/vect/aligned-section-anchors-vect-73.c b/gcc/testsuite/gcc.dg/vect/aligned-section-anchors-vect-73.c
>> >> new file mode 100644
>> >> index 0000000..74eaed8
>> >> --- /dev/null
>> >> +++ b/gcc/testsuite/gcc.dg/vect/aligned-section-anchors-vect-73.c
>> >> @@ -0,0 +1,25 @@
>> >> +/* { dg-do compile } */
>> >> +/* { dg-require-effective-target section_anchors } */
>> >> +/* { dg-require-effective-target vect_int } */
>> >> +
>> >> +#define N 32
>> >> +
>> >> +/* Clone of section-anchors-vect-70.c with foo() having -fno-tree-loop-vectorize.  */
>> >> +
>> >> +static struct A {
>> >> +  int p1, p2;
>> >> +  int e[N];
>> >> +} a, b, c;
>> >> +
>> >> +__attribute__((optimize("-fno-tree-loop-vectorize")))
>> >> +int foo(void)
>> >> +{
>> >> +  for (int i = 0; i < N; i++)
>> >> +    a.e[i] = b.e[i] + c.e[i];
>> >> +
>> >> +   return a.e[0];
>> >> +}
>> >> +
>> >> +/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target aarch64*-*-* } } } */
>> >> +/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target powerpc64*-*-* } } } */
>> >> +/* { dg-final { scan-ipa-dump-times "Increasing alignment of decl" 0 "increase_alignment" { target arm*-*-* } } } */
>> >> diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h
>> >> index 36299a6..d36aa1d 100644
>> >> --- a/gcc/tree-pass.h
>> >> +++ b/gcc/tree-pass.h
>> >> @@ -483,7 +483,7 @@ extern simple_ipa_opt_pass *make_pass_local_optimization_passes (gcc::context *c
>> >>
>> >>  extern ipa_opt_pass_d *make_pass_ipa_whole_program_visibility (gcc::context
>> >>                                                              *ctxt);
>> >> -extern simple_ipa_opt_pass *make_pass_ipa_increase_alignment (gcc::context
>> >> +extern ipa_opt_pass_d *make_pass_ipa_increase_alignment (gcc::context
>> >>                                                             *ctxt);
>> >>  extern ipa_opt_pass_d *make_pass_ipa_inline (gcc::context *ctxt);
>> >>  extern simple_ipa_opt_pass *make_pass_ipa_free_lang_data (gcc::context *ctxt);
>> >> diff --git a/gcc/tree-vectorizer.c b/gcc/tree-vectorizer.c
>> >> index 2669813..d34e560 100644
>> >> --- a/gcc/tree-vectorizer.c
>> >> +++ b/gcc/tree-vectorizer.c
>> >> @@ -899,6 +899,34 @@ get_vec_alignment_for_type (tree type)
>> >>    return (alignment > TYPE_ALIGN (type)) ? alignment : 0;
>> >>  }
>> >>
>> >> +/* Return true if alignment should be increased for this vnode.
>> >> +   This is done if every function that references/referring to vnode
>> >> +   has flag_tree_loop_vectorize and flag_section_anchors set.  */
>> >> +
>> >> +static bool
>> >> +increase_alignment_p (varpool_node *vnode)
>> >> +{
>> >> +  ipa_ref *ref;
>> >> +
>> >> +  for (int i = 0; vnode->iterate_reference (i, ref); i++)
>> >> +    if (cgraph_node *cnode = dyn_cast<cgraph_node *> (ref->referred))
>> >> +      {
>> >> +     struct cl_optimization *opts = opts_for_fn (cnode->decl);
>> >> +     if (!opts->x_flag_tree_loop_vectorize)
>> >> +       return false;
>> >> +      }
>> >> +
>> >> +  for (int i = 0; vnode->iterate_referring (i, ref); i++)
>> >> +    if (cgraph_node *cnode = dyn_cast<cgraph_node *> (ref->referring))
>> >> +      {
>> >> +     struct cl_optimization *opts = opts_for_fn (cnode->decl);
>> >> +     if (!opts->x_flag_tree_loop_vectorize)
>> >> +       return false;
>> >> +      }
>> >> +
>> >> +  return true;
>> >> +}
>> >> +
>> >>  /* Entry point to increase_alignment pass.  */
>> >>  static unsigned int
>> >>  increase_alignment (void)
>> >> @@ -916,7 +944,8 @@ increase_alignment (void)
>> >>
>> >>        if ((decl_in_symtab_p (decl)
>> >>         && !symtab_node::get (decl)->can_increase_alignment_p ())
>> >> -       || DECL_USER_ALIGN (decl) || DECL_ARTIFICIAL (decl))
>> >> +       || DECL_USER_ALIGN (decl) || DECL_ARTIFICIAL (decl)
>> >> +       || !increase_alignment_p (vnode))
>> >>       continue;
>> >>
>> >>        alignment = get_vec_alignment_for_type (TREE_TYPE (decl));
>> >> @@ -938,7 +967,7 @@ namespace {
>> >>
>> >>  const pass_data pass_data_ipa_increase_alignment =
>> >>  {
>> >> -  SIMPLE_IPA_PASS, /* type */
>> >> +  IPA_PASS, /* type */
>> >>    "increase_alignment", /* name */
>> >>    OPTGROUP_LOOP | OPTGROUP_VEC, /* optinfo_flags */
>> >>    TV_IPA_OPT, /* tv_id */
>> >> @@ -949,17 +978,26 @@ const pass_data pass_data_ipa_increase_alignment =
>> >>    0, /* todo_flags_finish */
>> >>  };
>> >>
>> >> -class pass_ipa_increase_alignment : public simple_ipa_opt_pass
>> >> +class pass_ipa_increase_alignment : public ipa_opt_pass_d
>> >>  {
>> >>  public:
>> >>    pass_ipa_increase_alignment (gcc::context *ctxt)
>> >> -    : simple_ipa_opt_pass (pass_data_ipa_increase_alignment, ctxt)
>> >> +    : ipa_opt_pass_d (pass_data_ipa_increase_alignment, ctxt,
>> >> +                        NULL, /* generate_summary  */
>> >> +                        NULL, /* write summary  */
>> >> +                        NULL, /* read summary  */
>> >> +                        NULL, /* write optimization summary  */
>> >> +                        NULL, /* read optimization summary  */
>> >> +                        NULL, /* stmt fixup  */
>> >> +                        0, /* function_transform_todo_flags_start  */
>> >> +                        NULL, /* transform function  */
>> >> +                        NULL )/* variable transform  */
>> >>    {}
>> >>
>> >>    /* opt_pass methods: */
>> >>    virtual bool gate (function *)
>> >>      {
>> >> -      return flag_section_anchors && flag_tree_loop_vectorize;
>> >> +      return flag_section_anchors != 0;
>> >>      }
>> >>
>> >>    virtual unsigned int execute (function *) { return increase_alignment (); }
>> >> @@ -968,7 +1006,7 @@ public:
>> >>
>> >>  } // anon namespace
>> >>
>> >> -simple_ipa_opt_pass *
>> >> +ipa_opt_pass_d *
>> >>  make_pass_ipa_increase_alignment (gcc::context *ctxt)
>> >>  {
>> >>    return new pass_ipa_increase_alignment (ctxt);
>> >
>>
>>
>
> --
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Attachment: ias2r-6.diff
Description: Text document