Re: [RFC/PATCH] vect: Recog mul_highpart pattern

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

on 2021/7/14 下午7:32, Richard Sandiford wrote:
> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>> Hi Richard,
>>
>> on 2021/7/14 下午4:38, Richard Sandiford wrote:
>>> "Kewen.Lin" <linkw@linux.ibm.com> writes:
>>>> gcc/ChangeLog:
>>>>
>>>> 	* internal-fn.c (first_commutative_argument): Add info for IFN_MULH.
>>>> 	* internal-fn.def (IFN_MULH): New internal function.
>>>> 	* tree-vect-patterns.c (vect_recog_mulhs_pattern): Add support to
>>>> 	recog normal multiply highpart as IFN_MULH.
>>>
>>> LGTM FWIW, although:
>>>
>>
>> Thanks for the review!
>>
>>>> @@ -2030,8 +2048,7 @@ vect_recog_mulhs_pattern (vec_info *vinfo,
>>>>    /* Check for target support.  */
>>>>    tree new_vectype = get_vectype_for_scalar_type (vinfo, new_type);
>>>>    if (!new_vectype
>>>> -      || !direct_internal_fn_supported_p
>>>> -	    (ifn, new_vectype, OPTIMIZE_FOR_SPEED))
>>>> +      || !direct_internal_fn_supported_p (ifn, new_vectype, OPTIMIZE_FOR_SPEED))
>>>>      return NULL;
>>>>  
>>>>    /* The IR requires a valid vector type for the cast result, even though
>>>> @@ -2043,8 +2060,8 @@ vect_recog_mulhs_pattern (vec_info *vinfo,
>>>>    /* Generate the IFN_MULHRS call.  */
>>>>    tree new_var = vect_recog_temp_ssa_var (new_type, NULL);
>>>>    tree new_ops[2];
>>>> -  vect_convert_inputs (vinfo, last_stmt_info, 2, new_ops, new_type,
>>>> -		       unprom_mult, new_vectype);
>>>> +  vect_convert_inputs (vinfo, last_stmt_info, 2, new_ops, new_type, unprom_mult,
>>>> +		       new_vectype);
>>>>    gcall *mulhrs_stmt
>>>>      = gimple_build_call_internal (ifn, 2, new_ops[0], new_ops[1]);
>>>>    gimple_call_set_lhs (mulhrs_stmt, new_var);
>>>
>>> …these changes look like formatting only.  (I guess it's down to whether
>>> or not the 80th column should be kept free for an “end of line+1” cursor.)
>>>
>>
>> Yeah, just for formatting, the formatting tool (clang-format) reformatted
>> them.  Thanks for the information on "end of line+1" cursor, I didn't know
>> that before.  I guess you prefer me to keep the original format?  If so I
>> will remove them when committing it.  I was thinking whether I should change
>> field ColumnLimit of my .clang-format to 79 to avoid this kind of case to
>> be caught by formatting tool again.  Hope reviewers won't nit-pick the exact
>> 80 column cases then. :)
> 
> TBH, 79 vs. 80 isn't normally something I'd worry about when reviewing
> new code.  But I know in the past people have asked for 79 to be used
> for the “end+1” reason, so I don't think we should “fix” existing code
> that honours the 79 limit so that it no longer does, especially when the
> lines surrounding the code aren't changing.
> 

Thanks for the explanation!  Agree.

> There's also a risk of yo-yo-ing if someone else is using clang-format
> and does have the limit set to 79 columns.
> 
> So yeah, I think it'd better to commit without the two hunks above.
> 

Will fix them.  Thanks for catching and explanations!

BR,
Kewen