Re: [PATCH] Fix PR94401 by considering reverse overrun

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi,

on 2020/4/2 下午4:28, Jakub Jelinek wrote:
> 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));
> 

Thanks Jakub!  I'll follow this by add () for ternary expression.
With manual added "()", clang-format can get below:

			tree offset
			  = (dataref_offset ? dataref_offset
					    : build_int_cst (ref_type, 0));

contrib/check_GNU_style.sh didn't complain this, I'm not sure whether
it's possible to add this kind of convention into contrib/clang-format.

>> +			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);
> 			  }
> 

Good suggestion, will update it.

BR,
Kewen