Re: move increase_alignment from simple to regular ipa pass

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

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.

Thanks,
Prathamesh
>
> Honza

Attachment: ias2r-7.diff
Description: Text document