> -----Original Message----- > From: Richard Sandiford [mailto:richard.sandif...@arm.com] > Sent: Saturday, August 29, 2020 2:31 AM > To: duanbo (C) <duan...@huawei.com> > Cc: GCC Patches <gcc-patches@gcc.gnu.org>; rguent...@suse.de > Subject: Re: [PATCH PR96757] aarch64: ICE during GIMPLE pass: vect > > "duanbo (C)" <duan...@huawei.com> writes: > > @@ -4395,6 +4395,40 @@ vect_recog_mask_conversion_pattern > (vec_info *vinfo, > > { > > tmp = vect_recog_temp_ssa_var (TREE_TYPE (rhs1), NULL); > > pattern_stmt = gimple_build_assign (tmp, rhs1); > > + tree rhs1_op0 = TREE_OPERAND (rhs1, 0); > > + tree rhs1_op1 = TREE_OPERAND (rhs1, 1); > > I think we also need to handle this case when picking the vector types and > deciding whether a pattern is needed. Otherwise it would be possible for > rhs1_op1 to be the only input that requires a different mask, and we'd > wrongly avoid creating a conversion for it. > > > + if (rhs1_op0 && rhs1_op1 > > + && (TREE_CODE (TREE_TYPE (rhs1_op0)) == BOOLEAN_TYPE) > > + && (TREE_CODE (TREE_TYPE (rhs1_op1)) == BOOLEAN_TYPE)) > > Minor formatting nit, but the brackets around the == are redundant (but are > still kept for multiline comparisons like the == below). > > > + { > > + tree rhs1_op0_type = integer_type_for_mask (rhs1_op0, vinfo); > > + tree rhs1_op1_type = integer_type_for_mask (rhs1_op1, vinfo); > > + enum tree_code rhs1_code = gimple_assign_rhs_code > (pattern_stmt); > > + if (rhs1_op0_type && rhs1_op1_type > > + && (!(TYPE_PRECISION (rhs1_op0_type) > > + == TYPE_PRECISION (rhs1_op1_type)))) > > More obvious as != rather than !(… == …). > > > + { > > + if (TYPE_PRECISION (rhs1_op0_type) > > + < TYPE_PRECISION (rhs1_op1_type)) > > + { > > + vectype2 > > + = get_mask_type_for_scalar_type (vinfo, > rhs1_op0_type); > > + if (vectype2) > > + rhs1_op1 = build_mask_conversion (vinfo, rhs1_op1, > > + vectype2, > stmt_vinfo); > > It isn't safe to ignore null returns like this. We should bail out instead. > > > + } > > + else > > + { > > + vectype2 > > + = get_mask_type_for_scalar_type (vinfo, > rhs1_op1_type); > > + if (vectype2) > > + rhs1_op0 = build_mask_conversion (vinfo, rhs1_op0, > > + vectype2, > stmt_vinfo); > > + } > > This seems to go for the narrower of the two precisions. Are you sure that's > always the optimal choice? Gut instinct says that we should move in the > direction of vectype1, since that's what the result will be converted to. > E.g. > maybe: > > - if both precisions are less than vectype1's precision or both are > more than vectype1's precision, pick the closest to vectype1. > > - otherwise, convert both inputs to vectype1 individually, rather than > converting the result to vectype1. > > I admit I haven't thought about it much though. > > > + pattern_stmt = gimple_build_assign (tmp, rhs1_code, > > + rhs1_op0, rhs1_op1); > > + } > > This statement is redundant with the one created above. I think instead we > should create the statement this way in all cases, even if rhs1_op0 and > rhs1_op1 don't change. That's effectively what the: > > gimple_build_assign (tmp, rhs1) > > does anyway. > > I'm going to be away next week so my next reply might be even slower than > usual, sorry. Would be happy for someone else to review in the meantime > though. :-) > > Thanks, > Richard
Sorry for the late reply. Thanks for your suggestions. I have modified accordingly. Attached please find the v1 patch. Bootstrap and tested on both aarch64 and x86 Linux platform, no new regression witnessed. Ok for trunk? Thanks, Duan bo
pr96757-v1.patch
Description: pr96757-v1.patch
