Hi Robin, Thanks for reviewing.
Cleanup up here is good, right now it's not really an insn_type but indeed just the number of operands. My original idea was to have an insn type and a mostly unified expander that performs all necessary operations depending on the insn_type. Just to give an idea of why it's called that way.+ rtx ops[RVV_BINOP_MASK] = {target, mask, target, op, sel}; + emit_vlmax_masked_mu_insn (icode, RVV_BINOP_MASK, ops);One of the ideas was that a function emit_vlmax_masked_mu_insn would already know that it's dealing with a mask and we would just pass something like RVV_BINOP. The other way would be to just have emit_vlmax_mu_insn or something and let the rest be deduced from the insn_type. Even the vlmax I intended to have mostly implicit but that somehow got lost during refactorings :) No need to change anything for now, just for perspective again.
I think the ideas of these two comments will be reflected in the next patch of refactoring emit_vlmax/nonvlmax_xxx functions (abstract several uniform types of RVV instructions), thanks a lot!
Would you mind renaming op_num (i.e. usually understood as operand_number) into num_ops or nops? (i.e. number of operands). That way we would be more in line of what the later expander functions do.
OK, rename op_num into num_ops.
I would actually prefer to keep "ops" because it's already clear from the function name that we work with a conditional function (and we don't have any other ops).
No problem.
We're already a bit inconsistent with how we pasds mask, merge and the source operands. Maybe we could also unify this a bit? I don't have a clear preference for either, though.+ rtx cond_ops[RVV_BINOP_MASK] = {dest, mask, merge, src1, src2};Here, the merge comes before the sources as well.+ rtx cond_ops[RVV_TERNOP_MASK] = {dest, mask, src1, src2, src3, merge};And here, the merge comes last. I realize this makes sense in the context of a ternary operation because the merge is always "real". As our vector patterns are similar, maybe we should use this ordering all the time?
Yes, the ternary merge is not placed in operand 2 like binop or unop, I discussed it with Juzhe and it could be unified, but it would change to the intrinsic part, so I would suggest to bring up a separate patch to unfied the operand order. The unfied order like this:
DEST, MASK, MERGE (this three operands fixed for most insns) OPS (the number can be 1, 2, 3) VL, TAIL_POLICY, MASK_POLICY, AVL_TYPE, ROUDING_MODE -- Best, Lehua
