on 2021/7/14 下午7:32, Richard Sandiford wrote: > "Kewen.Lin" <li...@linux.ibm.com> writes: >> Hi Richard, >> >> on 2021/7/14 下午4:38, Richard Sandiford wrote: >>> "Kewen.Lin" <li...@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
