Re: [PATCH] Fix PR94401 by considering reverse overrun

[Jakub Jelinek <jakub@redhat.com>]

Hi!

On Thu, Apr 02, 2020 at 03:15:42PM +0800, Kewen.Lin via Gcc-patches wrote:

Just formatting nits, not commenting on what the actual patch does.

> --- a/gcc/tree-vect-stmts.c
> +++ b/gcc/tree-vect-stmts.c
> @@ -9590,11 +9590,20 @@ vectorizable_load (stmt_vec_info stmt_info, gimple_stmt_iterator *gsi,
>  			    if (new_vtype != NULL_TREE)
>  			      ltype = half_vtype;
>  			  }
> +			tree offset = dataref_offset
> +					? dataref_offset
> +					: build_int_cst (ref_type, 0);

The above is misformatted.  The ? and : shouldn't be indented further than
the dataref_offset, but usually e.g. for the sake of emacs we add ()s around
the expression in this case.  So:
			tree offset = (dataref_offset
				       ? dataref_offset
				       : build_int_cst (ref_type, 0));
or
			tree offset
			  = (dataref_offset
			     ? dataref_offset : build_int_cst (ref_type, 0));

> +			if (ltype != vectype
> +			    && memory_access_type == VMAT_CONTIGUOUS_REVERSE)
> +			  offset = size_binop (
> +			    PLUS_EXPR,
> +			    build_int_cst (ref_type,
> +					   DR_GROUP_GAP (first_stmt_info)
> +					     * tree_to_uhwi (
> +					       TYPE_SIZE_UNIT (elem_type))),
> +			    offset);

Again, no reason to indent * by 2 columns from DR_GROUP_GAP.  But also all
the (s at the end of line and randomly indented arguments look ugly.
I'd recommend temporaries, e.g. like (perhaps with different names of
temporaries, so that they don't shadow anything):

			  {
			    unsigned HOST_WIDE_INT gap
			      = DR_GROUP_GAP (first_stmt_info);
			    gap *= tree_to_uhwi (TYPE_SIZE_UNIT (elem_type));
			    tree gapcst = build_int_cst (ref_type, gap);
			    offset = size_binop (PLUS_EXPR, offset, gapcst);
			  }

	Jakub