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 3E0763857C65 for ; Thu, 24 Sep 2020 11:55:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 3E0763857C65 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=richard.sandiford@arm.com 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 D2E6F113E; Thu, 24 Sep 2020 04:55:46 -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 5F48F3F73B; Thu, 24 Sep 2020 04:55:46 -0700 (PDT) From: Richard Sandiford To: "duanbo \(C\)" Mail-Followup-To: "duanbo \(C\)" , GCC Patches , richard.sandiford@arm.com Cc: GCC Patches Subject: Re: [PATCH PR96757] aarch64: ICE during GIMPLE pass: vect References: Date: Thu, 24 Sep 2020 12:55:44 +0100 In-Reply-To: (duanbo's message of "Sat, 19 Sep 2020 08:06:50 +0000") 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=-7.1 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Thu, 24 Sep 2020 11:55:49 -0000 Hi, "duanbo (C)" writes: > Sorry for the late reply. My time to apologise for the late reply. > Thanks for your suggestions. I have modified accordingly. > Attached please find the v1 patch.=20 Thanks, the logic to choose which precision we pick looks good. But I think the build_mask_conversions should be deferred until after we've decided to make the transform. So=E2=80=A6 > @@ -4340,16 +4342,91 @@ vect_recog_mask_conversion_pattern (vec_info *vin= fo, >=20=20 > it is better for b1 and b2 to use the mask type associated > with int elements rather bool (byte) elements. */ > - rhs1_type =3D integer_type_for_mask (TREE_OPERAND (rhs1, 0), vinfo); > - if (!rhs1_type) > - rhs1_type =3D TREE_TYPE (TREE_OPERAND (rhs1, 0)); > + rhs1_op0 =3D TREE_OPERAND (rhs1, 0); > + rhs1_op1 =3D TREE_OPERAND (rhs1, 1); > + if (!rhs1_op0 || !rhs1_op1) > + return NULL; > + rhs1_op0_type =3D integer_type_for_mask (rhs1_op0, vinfo); > + rhs1_op1_type =3D integer_type_for_mask (rhs1_op1, vinfo); > + > + if (!rhs1_op0_type && !rhs1_op1_type) > + { > + rhs1_type =3D TREE_TYPE (rhs1_op0); > + vectype2 =3D get_mask_type_for_scalar_type (vinfo, rhs1_type); =E2=80=A6here we should just be able to set rhs1_type, and leave vectype2 to the code below. > + } > + else if (!rhs1_op0_type && rhs1_op1_type) > + { > + rhs1_type =3D TREE_TYPE (rhs1_op0); > + vectype2 =3D get_mask_type_for_scalar_type (vinfo, rhs1_type); > + if (!vectype2) > + return NULL; > + rhs1_op1 =3D build_mask_conversion (vinfo, rhs1_op1, > + vectype2, stmt_vinfo); > + } > + else if (rhs1_op0_type && !rhs1_op1_type) > + { > + rhs1_type =3D TREE_TYPE (rhs1_op1); > + vectype2 =3D get_mask_type_for_scalar_type (vinfo, rhs1_type); > + if (!vectype2) > + return NULL; > + rhs1_op0 =3D build_mask_conversion (vinfo, rhs1_op0, > + vectype2, stmt_vinfo); Same for these two. > + } > + else if (TYPE_PRECISION (rhs1_op0_type) > + !=3D TYPE_PRECISION (rhs1_op1_type)) > + { > + int tmp1 =3D (int)TYPE_PRECISION (rhs1_op0_type) > + - (int)TYPE_PRECISION (TREE_TYPE (lhs)); > + int tmp2 =3D (int)TYPE_PRECISION (rhs1_op1_type) > + - (int)TYPE_PRECISION (TREE_TYPE (lhs)); > + if ((tmp1 > 0 && tmp2 > 0)||(tmp1 < 0 && tmp2 < 0)) Minor formatting nit, sorry, but: GCC style is to put a space after (int) and on either side of ||. Might be good to use the same numbering as the operands: tmp0 and tmp1 instead of tmp1 and tmp2. > + { > + if (abs (tmp1) > abs (tmp2)) > + { > + vectype2 =3D get_mask_type_for_scalar_type (vinfo, > + rhs1_op1_type); > + if (!vectype2) > + return NULL; > + rhs1_op0 =3D build_mask_conversion (vinfo, rhs1_op0, > + vectype2, stmt_vinfo); > + } > + else > + { > + vectype2 =3D get_mask_type_for_scalar_type (vinfo, > + rhs1_op0_type); > + if (!vectype2) > + return NULL; > + rhs1_op1 =3D build_mask_conversion (vinfo, rhs1_op1, > + vectype2, stmt_vinfo); > + } > + rhs1_type =3D integer_type_for_mask (rhs1_op0, vinfo); Here I think we can just go with rhs1_type =3D rhs1_op1_type if abs (tmp1) > abs (tmp2) (i.e. op1 is closer to the final type than op0) and rhs1_op0_type otherwise. > + } > + else > + { > + rhs1_op0 =3D build_mask_conversion (vinfo, rhs1_op0, > + vectype1, stmt_vinfo); > + rhs1_op1 =3D build_mask_conversion (vinfo, rhs1_op1, > + vectype1, stmt_vinfo); > + rhs1_type =3D integer_type_for_mask (rhs1_op0, vinfo); > + if (!rhs1_type) > + return NULL; > + vectype2 =3D get_mask_type_for_scalar_type (vinfo, rhs1_type); and here I think rhs1_type can be: build_nonstandard_integer_type (TYPE_PRECISION (lhs_type), 1); > + } > + } > + else > + { > + rhs1_type =3D integer_type_for_mask (rhs1_op0, vinfo); > + if (!rhs1_type) > + return NULL; > + vectype2 =3D get_mask_type_for_scalar_type (vinfo, rhs1_type); Here either rhs1_op0_type or rhs1_op1_type should be OK. > + } > + tmp =3D build2 (TREE_CODE (rhs1), TREE_TYPE (rhs1), rhs1_op0, rhs1_op= 1); > + rhs1 =3D tmp; > } > else > return NULL; >=20=20 > - vectype2 =3D get_mask_type_for_scalar_type (vinfo, rhs1_type); > - > - if (!vectype1 || !vectype2) > + if (!vectype2) > return NULL; >=20=20 > /* Continue if a conversion is needed. Also continue if we have With those changes, I think we would need to extend: /* Continue if a conversion is needed. Also continue if we have a comparison whose vector type would normally be different from VECTYPE2 when considered in isolation. In that case we'll replace the comparison with an SSA name (so that we can record its vector type) and behave as though the comparison was an SSA name from the outset. */ if (known_eq (TYPE_VECTOR_SUBPARTS (vectype1), TYPE_VECTOR_SUBPARTS (vectype2)) && (TREE_CODE (rhs1) =3D=3D SSA_NAME || rhs1_type =3D=3D TREE_TYPE (TREE_OPERAND (rhs1, 0)))) =E2=80=A6the condition on the last line so that it also checks whether rhs1_op0_type and rhs1_op1_type are null. If they aren't null, we need to fall down to the code below. return NULL; And then here we create the build_mask_conversions to vectype2, like I think you did in the first patch, but reusing the rhs1_op0_type and rhs1_op1_type from above, and without changing vectype2: /* If rhs1 is a comparison we need to move it into a separate statement. */ if (TREE_CODE (rhs1) !=3D SSA_NAME) { tmp =3D vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL); pattern_stmt =3D gimple_build_assign (tmp, rhs1); rhs1 =3D tmp; append_pattern_def_seq (vinfo, stmt_vinfo, pattern_stmt, vectype2, rhs1_type); } Hope that works/makes sense. Please let me know if it doesn't. Thanks, Richard