Re: [PATCH, vec-tails 04/10] Add masking cost

[Jeff Law <law@redhat.com>]

On 05/19/2016 01:40 PM, Ilya Enkovich wrote:
> Hi,
>
> This patch extends vectorizer cost model to include masking cost by
> adding new cost model locations and new target hook to compute
> masking cost.
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2016-05-19  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	* config/i386/i386.c (ix86_init_cost): Extend costs array.
> 	(ix86_add_stmt_masking_cost): New.
> 	(ix86_finish_cost): Add masking_prologue_cost and masking_body_cost
> 	args.
> 	(TARGET_VECTORIZE_ADD_STMT_MASKING_COST): New.
> 	* config/i386/i386.h (TARGET_INCREASE_MASK_STORE_COST): New.
> 	* config/i386/x86-tune.def (X86_TUNE_INCREASE_MASK_STORE_COST): New.
> 	* config/rs6000/rs6000.c (_rs6000_cost_data): Extend cost array.
> 	(rs6000_init_cost): Initialize new cost elements.
> 	(rs6000_finish_cost): Add masking_prologue_cost and masking_body_cost.
> 	* config/spu/spu.c (spu_init_cost): Extend costs array.
> 	(spu_finish_cost): Add masking_prologue_cost and masking_body_cost args.
> 	* doc/tm.texi.in (TARGET_VECTORIZE_ADD_STMT_MASKING_COST): New.
> 	* doc/tm.texi: Regenerated.
> 	* target.def (add_stmt_masking_cost): New.
> 	(finish_cost): Add masking_prologue_cost and masking_body_cost args.
> 	* target.h (enum vect_cost_for_stmt): Add vector_mask_load and
> 	vector_mask_store.
> 	(enum vect_cost_model_location): Add vect_masking_prologue
> 	and vect_masking_body.
> 	* targhooks.c (default_builtin_vectorization_cost): Support
> 	vector_mask_load and vector_mask_store.
> 	(default_init_cost): Extend costs array.
> 	(default_add_stmt_masking_cost): New.
> 	(default_finish_cost): Add masking_prologue_cost and masking_body_cost
> 	args.
> 	* targhooks.h (default_add_stmt_masking_cost): New.
> 	* tree-vect-loop.c (vect_estimate_min_profitable_iters): Adjust
> 	finish_cost call.
> 	* tree-vect-slp.c (vect_bb_vectorization_profitable_p): Likewise.
> 	* tree-vectorizer.h (add_stmt_masking_cost): New.
> 	(finish_cost): Add masking_prologue_cost and masking_body_cost args.
>
>
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 9f62089..6c2c364 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -53932,8 +53932,12 @@ ix86_spill_class (reg_class_t rclass, machine_mode mode)
>  static void *
>  ix86_init_cost (struct loop *)
>  {
> -  unsigned *cost = XNEWVEC (unsigned, 3);
> -  cost[vect_prologue] = cost[vect_body] = cost[vect_epilogue] = 0;
> +  unsigned *cost = XNEWVEC (unsigned, 5);
> +  cost[vect_prologue] = 0;
> +  cost[vect_body]     = 0;
> +  cost[vect_epilogue] = 0;
> +  cost[vect_masking_prologue] = 0;
> +  cost[vect_masking_body] = 0;
>    return cost;
Trivial nit -- no need or desire to use whitespace to line up the 
initializers.   It looks like others may have done this in the 
duplicated instances of finish_cost. But we shouldn't propagate that 
mistake into the init_cost hooks ;-)


@@ -1115,8 +1117,12 @@ default_get_mask_mode (unsigned nunits, unsigned 
vector_size)
>  void *
>  default_init_cost (struct loop *loop_info ATTRIBUTE_UNUSED)
>  {
> -  unsigned *cost = XNEWVEC (unsigned, 3);
> -  cost[vect_prologue] = cost[vect_body] = cost[vect_epilogue] = 0;
> +  unsigned *cost = XNEWVEC (unsigned, 5);
> +  cost[vect_prologue] = 0;
> +  cost[vect_body]     = 0;
> +  cost[vect_epilogue] = 0;
> +  cost[vect_masking_prologue] = 0;
> +  cost[vect_masking_body] = 0;
>    return cost;
Here too.  There's others.  I won't point them all out.  Please double 
check for this nit in any added code.  You don't have to go back and fix 
existing problems of this nature.

I don't see anything here I really object to -- Richi and I may disagree 
on the compute-costs once in a single scan vs restarting the scan.  If 
Richi feels strongly about restarting for some reason, I'll defer to him 
-- he's done more work in the vectorizer than myself.

I'd suggest taking another stab at the docs for the hooks based on 
Richi's question about whether or not the hook returns the cost of hte 
masked statement or the cost of masking the statement.

jeff