> -----Original Message----- > From: Robin Dapp <[email protected]> > Sent: 23 September 2026 16:36 > To: Tamar Christina <[email protected]>; [email protected] > Cc: nd <[email protected]>; [email protected]; [email protected] > Subject: Re: [patch 7/14]middle-end: add conditional vector compare IFNs > > > +static bool > > +expand_cond_vec_cmp_1_p (optab optab, tree_code tcode, tree mask, > tree op0, > > + tree len = NULL_TREE, tree bias = NULL_TREE) > > +{ > > + tree lhs_type = TREE_TYPE (mask); > > + tree op_type = TREE_TYPE (op0); > > + machine_mode mask_mode = TYPE_MODE (lhs_type); > > + machine_mode op_mode = TYPE_MODE (op_type); > > + > > + gcc_assert (TREE_CODE_CLASS (tcode) == tcc_comparison); > > + > > + insn_code icode = convert_optab_handler (optab, op_mode, > mask_mode); > > + if (icode == CODE_FOR_nothing) > > + return false; > > + > > + enum rtx_code rcode = get_rtx_code_1 (tcode, TYPE_UNSIGNED > (op_type)); > > + if (rcode == UNKNOWN) > > + return false; > > + > > + rtx target = alloca_raw_REG (mask_mode, LAST_VIRTUAL_REGISTER); > > + if (!insn_operand_matches (icode, 0, target)) > > + return false; > > + > > + rtx pred = alloca_raw_REG (mask_mode, LAST_VIRTUAL_REGISTER + 1); > > + if (!insn_operand_matches (icode, 2, pred)) > > + return false; > > + > > + rtx reg1 = alloca_raw_REG (op_mode, LAST_VIRTUAL_REGISTER + 2); > > + rtx reg2 = alloca_raw_REG (op_mode, LAST_VIRTUAL_REGISTER + 3); > > + rtx test = alloca_rtx_fmt_ee (rcode, mask_mode, reg1, reg2); > > + if (!insn_operand_matches (icode, 1, test)) > > + return false; > > + > > + if (len) > > + { > > + gcc_assert (bias); > > + machine_mode len_mode = insn_data[(int) icode].operand[5].mode; > > + > > + if (len_mode == VOIDmode) > > + len_mode = TYPE_MODE (TREE_TYPE (len)); > > + rtx len_rtx = alloca_raw_REG (len_mode, LAST_VIRTUAL_REGISTER + 4); > > + if (!insn_operand_matches (icode, 5, len_rtx)) > > + return false; > > + > > + if (!tree_fits_shwi_p (bias) > > + || !insn_operand_matches (icode, 6, GEN_INT (tree_to_shwi > (bias)))) > > + return false; > > + } > > + > > + return true; > > +} > > I'm a bit surprised to see all these checks. I don't have a strong > reason not to do them but still wonder why here when we usually only > check one or two operands. Also, at least the bias I wouldn't validate > here because it is checked and tied down before already.
Yeah, But I didn't want to perform the transform from VEC_COND_* Into VEC_COND_CMP without checking that the transformation is valid which is why I check all operands. I understand that BIAS will likely never be so large that a truncate would be an issue, but the intention Is to just carry over the type you had before. So if during vect you said ok to HImode, the compare needs to accept HImode. This seemed reasonable to me but happy to relax it is you think It's too strict. > > Also, I believe the implementation right now implicitly requires a zero > else value, at least for the mask-inactive elements. For elements past > the length the else can be undefined as every consumer of the comparison > result will be length masked again. Ignoring the len/tail, I still > think cond_vec_cmp/cond_len... needs an else value. I would not do all > the discovery and workarounds that we have for masked load's else > operands but instead just force it to zero and have targets deal with > it, so no extra work for SVE just for RVV. Is this true though. X = COND_LEN_VEC_CMP (...) if (x != 0) ... The consumer isn't LEN masked, at least in GIMPLE. So having a non-zero else value makes this undefined. Sure if you implement vec_cbranch_* you'd eventually be LEN masked after expand, but in GIMPLE the operation is invalid and if you don't implement vec_cbranch_* you'd also potentially get wrong code. > > I realize this is very bike-sheddy... as we're likely not going to use > COND_LEN_VEC_CMP for RVV except for fp-trapping in which case the else > value again doesn't matter. And SVE always zeros. > But if we don't specify an else value this means the else is kind of > undefined, and, strictly speaking, we can't do the optimization in > isel/match. > As the above, I think the else has to be zero, because gimple consumers aren't always a conditional operation. But the idea was that you'd use vec_cbranch_* and cond_len_vec_cmp_* together to get the semantics you wanted. Would this not work for you? Thanks, Tamar > -- > Regards > Robin
