From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 2303439730FD for ; Wed, 14 Jul 2021 11:32:27 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 2303439730FD Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BB6D8D6E; Wed, 14 Jul 2021 04:32:26 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E6FC93F774; Wed, 14 Jul 2021 04:32:25 -0700 (PDT) From: Richard Sandiford To: "Kewen.Lin" Mail-Followup-To: "Kewen.Lin" , Richard Biener , GCC Patches , Segher Boessenkool , Bill Schmidt , richard.sandiford@arm.com Cc: Richard Biener , GCC Patches , Segher Boessenkool , Bill Schmidt Subject: Re: [RFC/PATCH] vect: Recog mul_highpart pattern References: <0b72fa77-a281-35e6-34e3-17cf26f18bc1@linux.ibm.com> Date: Wed, 14 Jul 2021 12:32:24 +0100 In-Reply-To: (Kewen Lin's message of "Wed, 14 Jul 2021 18:02:01 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 14 Jul 2021 11:32:28 -0000 "Kewen.Lin" writes: > Hi Richard, > > on 2021/7/14 =E4=B8=8B=E5=8D=884:38, Richard Sandiford wrote: >> "Kewen.Lin" 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. >>=20 >> LGTM FWIW, although: >>=20 > > Thanks for the review! > >>> @@ -2030,8 +2048,7 @@ vect_recog_mulhs_pattern (vec_info *vinfo, >>> /* Check for target support. */ >>> tree new_vectype =3D 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_F= OR_SPEED)) >>> return NULL; >>>=20=20 >>> /* The IR requires a valid vector type for the cast result, even tho= ugh >>> @@ -2043,8 +2060,8 @@ vect_recog_mulhs_pattern (vec_info *vinfo, >>> /* Generate the IFN_MULHRS call. */ >>> tree new_var =3D 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, un= prom_mult, >>> + new_vectype); >>> gcall *mulhrs_stmt >>> =3D gimple_build_call_internal (ifn, 2, new_ops[0], new_ops[1]); >>> gimple_call_set_lhs (mulhrs_stmt, new_var); >>=20 >> =E2=80=A6these changes look like formatting only. (I guess it's down to= whether >> or not the 80th column should be kept free for an =E2=80=9Cend of line+1= =E2=80=9D cursor.) >>=20 > > 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 cha= nge > 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 ex= act > 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 =E2=80=9Cend+1=E2=80=9D reason, so I don't think we should =E2=80= =9Cfix=E2=80=9D existing code that honours the 79 limit so that it no longer does, especially when the lines surrounding the code aren't changing. 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. Thanks, Richard