[PATCH] Fix PR94401 by considering reverse overrun

Jakub Jelinek jakub@redhat.com
Thu Apr 2 08:28:34 GMT 2020


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



More information about the Gcc-patches mailing list