move increase_alignment from simple to regular ipa pass

On 14 June 2016 at 18:31, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> On 13 June 2016 at 16:13, Jan Hubicka <hubicka@ucw.cz> wrote:

>>> diff --git a/gcc/cgraph.h b/gcc/cgraph.h

>>> index ecafe63..41ac408 100644

>>> --- a/gcc/cgraph.h

>>> +++ b/gcc/cgraph.h

>>> @@ -1874,6 +1874,9 @@ public:

>>>       if we did not do any inter-procedural code movement.  */

>>>    unsigned used_by_single_function : 1;

>>>

>>> +  /* Set if -fsection-anchors is set.  */

>>> +  unsigned section_anchor : 1;

>>> +

>>>  private:

>>>    /* Assemble thunks and aliases associated to varpool node.  */

>>>    void assemble_aliases (void);

>>> diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c

>>> index 4bfcad7..e75d5c0 100644

>>> --- a/gcc/cgraphunit.c

>>> +++ b/gcc/cgraphunit.c

>>> @@ -800,6 +800,9 @@ varpool_node::finalize_decl (tree decl)

>>>       it is available to notice_global_symbol.  */

>>>    node->definition = true;

>>>    notice_global_symbol (decl);

>>> +

>>> +  node->section_anchor = flag_section_anchors;

>>> +

>>>    if (TREE_THIS_VOLATILE (decl) || DECL_PRESERVE_P (decl)

>>>        /* Traditionally we do not eliminate static variables when not

>>>        optimizing and when not doing toplevel reoder.  */

>>> diff --git a/gcc/common.opt b/gcc/common.opt

>>> index f0d7196..e497795 100644

>>> --- a/gcc/common.opt

>>> +++ b/gcc/common.opt

>>> @@ -1590,6 +1590,10 @@ fira-algorithm=

>>>  Common Joined RejectNegative Enum(ira_algorithm) Var(flag_ira_algorithm) Init(IRA_ALGORITHM_CB) Optimization

>>>  -fira-algorithm=[CB|priority] Set the used IRA algorithm.

>>>

>>> +fipa-increase_alignment

>>> +Common Report Var(flag_ipa_increase_alignment) Init(0) Optimization

>>> +Option to gate increase_alignment ipa pass.

>>> +

>>>  Enum

>>>  Name(ira_algorithm) Type(enum ira_algorithm) UnknownError(unknown IRA algorithm %qs)

>>>

>>> @@ -2133,7 +2137,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/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c

>>> index a0db3a4..1482566 100644

>>> --- a/gcc/config/aarch64/aarch64.c

>>> +++ b/gcc/config/aarch64/aarch64.c

>>> @@ -8252,6 +8252,8 @@ aarch64_override_options (void)

>>>

>>>    aarch64_register_fma_steering ();

>>>

>>> +  /* Enable increase_alignment pass.  */

>>> +  flag_ipa_increase_alignment = 1;

>>

>> I would rather enable it always on targets that do support anchors.

> AFAIK aarch64 supports section anchors.

>>> diff --git a/gcc/lto/lto-symtab.c b/gcc/lto/lto-symtab.c

>>> index ce9e146..7f09f3a 100644

>>> --- a/gcc/lto/lto-symtab.c

>>> +++ b/gcc/lto/lto-symtab.c

>>> @@ -342,6 +342,13 @@ lto_symtab_merge (symtab_node *prevailing, symtab_node *entry)

>>>       The type compatibility checks or the completing of types has properly

>>>       dealt with most issues.  */

>>>

>>> +  /* ??? is this assert necessary ?  */

>>> +  varpool_node *v_prevailing = dyn_cast<varpool_node *> (prevailing);

>>> +  varpool_node *v_entry = dyn_cast<varpool_node *> (entry);

>>> +  gcc_assert (v_prevailing && v_entry);

>>> +  /* section_anchor of prevailing_decl wins.  */

>>> +  v_entry->section_anchor = v_prevailing->section_anchor;

>>> +

>> Other flags are merged in lto_varpool_replace_node so please move this there.

> Ah indeed, thanks for the pointers.

> I wonder though if we need to set

> prevailing_node->section_anchor = vnode->section_anchor ?

> IIUC, the function merges flags from vnode into prevailing_node

> and removes vnode. However we want prevailing_node->section_anchor

> to always take precedence.

>>> +/* 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 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;

>>> +      }

>>

>> If you take address of function that has vectorizer enabled probably doesn't

>> imply need to increase alignment of that var. So please drop the loop.

>>

>> You only want function that read/writes or takes address of the symbol. But

>> onthe other hand, you need to walk all aliases of the symbol by

>> call_for_symbol_and_aliases

>>> +

>>> +  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)

>>> @@ -914,9 +942,12 @@ increase_alignment (void)

>>>        tree decl = vnode->decl;

>>>        unsigned int alignment;

>>>

>>> -      if ((decl_in_symtab_p (decl)

>>> -       && !symtab_node::get (decl)->can_increase_alignment_p ())

>>> -       || DECL_USER_ALIGN (decl) || DECL_ARTIFICIAL (decl))

>>> +      if (!vnode->section_anchor

>>> +       || (decl_in_symtab_p (decl)

>>> +           && !symtab_node::get (decl)->can_increase_alignment_p ())

>>> +       || DECL_USER_ALIGN (decl)

>>> +       || DECL_ARTIFICIAL (decl)

>>> +       || !increase_alignment_p (vnode))

>>

>> Incrementally we probably should do more testing whether the variable looks like

>> someting that can be vectorized, i.e. it contains array, has address taken or the

>> accesses are array accesses within loop.

>> This can be done by the analysis phase of the IPA pass inspecting the function

>> bodies.

> Thanks, I will try to check for array accesses are within a loop in

> followup patch.

> I was wondering if we could we treat a homogeneous global struct

> (having members of one type),

> as a global array of that type and increase it's alignment if required ?

>>

>> I think it is important waste to bump up everything including error messages etc.

>> At least on i386 the effect on firefox datasegment of various alignment setting is

>> very visible.

> Um for a start, would it be OK to check if all functions referencing variable

> have attribute noreturn, and in that case we skip increasing the alignment ?

> I suppose that error functions would be having attribute noreturn set ?

>>

>> Looks OK to me otherwise. please send updated patch.

> I have done the changes in the attached patch (stage-1 built).

> I am not sure what to return from the callback function and

> arbitrarily chose to return true.

Hi,
In this version (stage-1 built), I added read/write summaries which
stream those variables
which we want to increase alignment for.

I have a few questions:

a) To check if global var is used inside a loop, I obtained
corresponding ipa_ref
and checked loop_containing_stmt (ref->stmt), however it returned non-NULL
even when ref->stmt was not inside a loop.
for instance:
int a[32];
int f() { int x = a[0]; return x; }
Um how to check if stmt is used inside a loop ?

b) Is it necessary during WPA to check if function has
flag_tree_vectorize_set since
during analysis phase in increase_alignment_generate_summary() I check
if cnode has flag_tree_loop_vectorize_set ?

c) In LTO_increase_alignment_section, the following is streamed:
i) a "count" of type uwhi, to represent number of variables
ii) decls corresponding to the variables
The variables are then obtained during read_summay using
symtab_node::get_create().
I suppose since decls for varpool_nodes would already be streamed in
LTO_section_decls, I was wondering if I
could somehow refer to the decls in that section to avoid duplicate
streaming of decls ?

Thanks,
Prathamesh
>

> Thanks,

> Prathamesh

>>

>> Honza

move increase_alignment from simple to regular ipa pass

Commit Message

Patch