Re: [RFC] Request for comments on ivopts patch

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Dec 15, 2015 at 12:06 AM, Steve Ellcey <sellcey@imgtec.com> wrote:
> On Mon, 2015-12-14 at 09:57 +0100, Richard Biener wrote:
>
>> I don't know enough to assess the effect of this but
>>
>>  1) not all archs can do auto-incdec so either the comment is misleading
>> or the test should probably be amended
>>  2) I wonder why with the comment ("during the loop") you exclude IP_NORMAL/END
>>
>> that said, the comment needs to explain the situation better.
>>
>> Of course all such patches need some code-gen effect investigation
>> on more than one arch.
>>
>> [I wonder if a IV cost adjust target hook makes sense at some point]
>>
>> Richard.
>
> I like the idea of a target hook to modify IV costs.  What do you think
> about this?  I had to move some structures from tree-ssa-loop-ivopts.c
> to tree-ssa-loop-ivopts.h in order to give a target hooks access to
> information on the IV candidates.

IMHO it's better to not have empty default implementations but do

if (targetm.adjust_iv_cand_cost)
  targetm.adjust_iv_cand_cost (...);

which saves an unconditional indirect call on most targets.

Generally I think we should prefer target independent heuristics if possible
or heuristics derived from target cost queries.  So this kind of hook
says we've given up (and it makes it too easy to change things just for
a single target).

Anyway, this is for next stage1 of course so there's some time to come up
with good ideas and do testing on more than one target.

Richard.

> Steve Ellcey
> sellcey@imgtec.com
>
>
> 2015-12-14  Steve Ellcey  <sellcey@imgtec.com>
>
>         * doc/tm.texi.in (TARGET_ADJUST_IV_CAND_COST): New target function.
>         * target.def (adjust_iv_cand_cost): New target function.
>         * target.h (struct iv_cand): New forward declaration.
>         * targhooks.c (default_adjust_iv_cand_cost): New default function.
>         * targhooks.h (default_adjust_iv_cand_cost): Ditto.
>         * tree-ssa-loop-ivopts.c (struct iv, enum iv_position, struct iv_cand)
>         Moved to tree-ssa-loop-ivopts.h.
>         (determine_iv_cost): Add call to targetm.adjust_iv_cand_cost.
>         * tree-ssa-loop-ivopts.h (struct iv, enum iv_position, struct iv_cand)
>         Copied here from tree-ssa-loop-ivopts.h.
>
> diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> index a0a0a81..1ad4c2d 100644
> --- a/gcc/doc/tm.texi.in
> +++ b/gcc/doc/tm.texi.in
> @@ -8221,6 +8221,8 @@ and the associated definitions of those functions.
>
>  @hook TARGET_OFFLOAD_OPTIONS
>
> +@hook TARGET_ADJUST_IV_CAND_COST
> +
>  @defmac TARGET_SUPPORTS_WIDE_INT
>
>  On older ports, large integers are stored in @code{CONST_DOUBLE} rtl
> diff --git a/gcc/target.def b/gcc/target.def
> index d754337..6bdcfcc 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -5846,6 +5846,12 @@ DEFHOOK
>   void, (tree *hold, tree *clear, tree *update),
>   default_atomic_assign_expand_fenv)
>
> +DEFHOOK
> +(adjust_iv_cand_cost,
> +"Allow target to modify the cost of a possible induction variable.",
> +void, (struct iv_cand *cand),
> + default_adjust_iv_cand_cost)
> +
>  /* Leave the boolean fields at the end.  */
>
>  /* True if we can create zeroed data by switching to a BSS section
> diff --git a/gcc/target.h b/gcc/target.h
> index ffc4d6a..6f55575 100644
> --- a/gcc/target.h
> +++ b/gcc/target.h
> @@ -139,6 +139,9 @@ struct ao_ref;
>  /* This is defined in tree-vectorizer.h.  */
>  struct _stmt_vec_info;
>
> +/* This is defined in tree-ivopts.h. */
> +struct iv_cand;
> +
>  /* These are defined in tree-vect-stmts.c.  */
>  extern tree stmt_vectype (struct _stmt_vec_info *);
>  extern bool stmt_in_inner_loop_p (struct _stmt_vec_info *);
> diff --git a/gcc/targhooks.c b/gcc/targhooks.c
> index dcf0863..0d0bbfc 100644
> --- a/gcc/targhooks.c
> +++ b/gcc/targhooks.c
> @@ -1961,4 +1961,10 @@ default_optab_supported_p (int, machine_mode, machine_mode, optimization_type)
>    return true;
>  }
>
> +/* Default implementation of TARGET_ADJUST_IV_CAND_COST.  */
> +void
> +default_adjust_iv_cand_cost (struct iv_cand *)
> +{
> +}
> +
>  #include "gt-targhooks.h"
> diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> index 47b5cfc..dd0481d 100644
> --- a/gcc/targhooks.h
> +++ b/gcc/targhooks.h
> @@ -253,4 +253,6 @@ extern void default_setup_incoming_vararg_bounds (cumulative_args_t ca ATTRIBUTE
>  extern bool default_optab_supported_p (int, machine_mode, machine_mode,
>                                        optimization_type);
>
> +extern void default_adjust_iv_cand_cost (struct iv_cand *);
> +
>  #endif /* GCC_TARGHOOKS_H */
> diff --git a/gcc/tree-ssa-loop-ivopts.c b/gcc/tree-ssa-loop-ivopts.c
> index 98dc451..5bfd232 100644
> --- a/gcc/tree-ssa-loop-ivopts.c
> +++ b/gcc/tree-ssa-loop-ivopts.c
> @@ -126,21 +126,6 @@ avg_loop_niter (struct loop *loop)
>    return niter;
>  }
>
> -/* Representation of the induction variable.  */
> -struct iv
> -{
> -  tree base;           /* Initial value of the iv.  */
> -  tree base_object;    /* A memory object to that the induction variable points.  */
> -  tree step;           /* Step of the iv (constant only).  */
> -  tree ssa_name;       /* The ssa name with the value.  */
> -  unsigned use_id;     /* The identifier in the use if it is the case.  */
> -  bool biv_p;          /* Is it a biv?  */
> -  bool have_use_for;   /* Do we already have a use for it?  */
> -  bool no_overflow;    /* True if the iv doesn't overflow.  */
> -  bool have_address_use;/* For biv, indicate if it's used in any address
> -                          type use.  */
> -};
> -
>  /* Per-ssa version information (induction variable descriptions, etc.).  */
>  struct version_info
>  {
> @@ -212,41 +197,6 @@ struct iv_use
>                         /* Const offset stripped from base address.  */
>  };
>
> -/* The position where the iv is computed.  */
> -enum iv_position
> -{
> -  IP_NORMAL,           /* At the end, just before the exit condition.  */
> -  IP_END,              /* At the end of the latch block.  */
> -  IP_BEFORE_USE,       /* Immediately before a specific use.  */
> -  IP_AFTER_USE,                /* Immediately after a specific use.  */
> -  IP_ORIGINAL          /* The original biv.  */
> -};
> -
> -/* The induction variable candidate.  */
> -struct iv_cand
> -{
> -  unsigned id;         /* The number of the candidate.  */
> -  bool important;      /* Whether this is an "important" candidate, i.e. such
> -                          that it should be considered by all uses.  */
> -  ENUM_BITFIELD(iv_position) pos : 8;  /* Where it is computed.  */
> -  gimple *incremented_at;/* For original biv, the statement where it is
> -                          incremented.  */
> -  tree var_before;     /* The variable used for it before increment.  */
> -  tree var_after;      /* The variable used for it after increment.  */
> -  struct iv *iv;       /* The value of the candidate.  NULL for
> -                          "pseudocandidate" used to indicate the possibility
> -                          to replace the final value of an iv by direct
> -                          computation of the value.  */
> -  unsigned cost;       /* Cost of the candidate.  */
> -  unsigned cost_step;  /* Cost of the candidate's increment operation.  */
> -  struct iv_use *ainc_use; /* For IP_{BEFORE,AFTER}_USE candidates, the place
> -                             where it is incremented.  */
> -  bitmap depends_on;   /* The list of invariants that are used in step of the
> -                          biv.  */
> -  struct iv *orig_iv;  /* The original iv if this cand is added from biv with
> -                          smaller type.  */
> -};
> -
>  /* Hashtable entry for common candidate derived from iv uses.  */
>  struct iv_common_cand
>  {
> @@ -5834,6 +5784,8 @@ determine_iv_cost (struct ivopts_data *data, struct iv_cand *cand)
>
>    cand->cost = cost;
>    cand->cost_step = cost_step;
> +
> +  targetm.adjust_iv_cand_cost (cand);
>  }
>
>  /* Determines costs of computation of the candidates.  */
> diff --git a/gcc/tree-ssa-loop-ivopts.h b/gcc/tree-ssa-loop-ivopts.h
> index 4495504..a3bc69e 100644
> --- a/gcc/tree-ssa-loop-ivopts.h
> +++ b/gcc/tree-ssa-loop-ivopts.h
> @@ -20,6 +20,57 @@ along with GCC; see the file COPYING3.  If not see
>  #ifndef GCC_TREE_SSA_LOOP_IVOPTS_H
>  #define GCC_TREE_SSA_LOOP_IVOPTS_H
>
> +
> +/* The position where the iv is computed.  */
> +enum iv_position
> +{
> +  IP_NORMAL,           /* At the end, just before the exit condition.  */
> +  IP_END,              /* At the end of the latch block.  */
> +  IP_BEFORE_USE,       /* Immediately before a specific use.  */
> +  IP_AFTER_USE,                /* Immediately after a specific use.  */
> +  IP_ORIGINAL          /* The original biv.  */
> +};
> +
> +/* Representation of the induction variable.  */
> +struct iv
> +{
> +  tree base;           /* Initial value of the iv.  */
> +  tree base_object;    /* A memory object to that the induction variable points.  */
> +  tree step;           /* Step of the iv (constant only).  */
> +  tree ssa_name;       /* The ssa name with the value.  */
> +  unsigned use_id;     /* The identifier in the use if it is the case.  */
> +  bool biv_p;          /* Is it a biv?  */
> +  bool have_use_for;   /* Do we already have a use for it?  */
> +  bool no_overflow;    /* True if the iv doesn't overflow.  */
> +  bool have_address_use;/* For biv, indicate if it's used in any address
> +                          type use.  */
> +};
> +
> +/* The induction variable candidate.  */
> +struct iv_cand
> +{
> +  unsigned id;         /* The number of the candidate.  */
> +  bool important;      /* Whether this is an "important" candidate, i.e. such
> +                          that it should be considered by all uses.  */
> +  ENUM_BITFIELD(iv_position) pos : 8;  /* Where it is computed.  */
> +  gimple *incremented_at;/* For original biv, the statement where it is
> +                          incremented.  */
> +  tree var_before;     /* The variable used for it before increment.  */
> +  tree var_after;      /* The variable used for it after increment.  */
> +  struct iv *iv;       /* The value of the candidate.  NULL for
> +                          "pseudocandidate" used to indicate the possibility
> +                          to replace the final value of an iv by direct
> +                          computation of the value.  */
> +  unsigned cost;       /* Cost of the candidate.  */
> +  unsigned cost_step;  /* Cost of the candidate's increment operation.  */
> +  struct iv_use *ainc_use; /* For IP_{BEFORE,AFTER}_USE candidates, the place
> +                             where it is incremented.  */
> +  bitmap depends_on;   /* The list of invariants that are used in step of the
> +                          biv.  */
> +  struct iv *orig_iv;  /* The original iv if this cand is added from biv with
> +                          smaller type.  */
> +};
> +
>  extern edge single_dom_exit (struct loop *);
>  extern void dump_iv (FILE *, struct iv *);
>  extern void dump_use (FILE *, struct iv_use *);
>
>
>
>