Hi Robin,
Thanks for these comments.
On 2023/8/31 17:16, Robin Dapp wrote:
Hi Lehua,
thanks, this definitely goes into the direction of what I had in mind and
simplifies a lot of the reduntant emit_... so it's good to have it.
I was too slow for a detailed response :) So just some high-level comments.
One thing I noticed is the overloading of "MASK_OP", we use it as
"operation on masks" i.e. an insn as well as "mask policy". IMHO we could
get rid of UNARY_MASK_OP and BINARY_MASK_OP and just decide whether to
add a mask policy depending on if all operands are masks (the same way we
did before).
I think I should change the marco name here form __MASK_OP to
__NORMAL_OP_MASK. MASK_OP means this is a insn operate on mask operands.
I lift up it here because I want to simplify the emit_insn method.
Related, and seeing that the MASK in UNARY_MASK_OP is somewhat redundant,
I feel we still mix concerns a bit. For example it is not obvious, from
the name at least, why a WIDEN_TERNARY_OP does not have a merge operand
and the decision making seems very "enum centered" now :D
As the above says, UNARY_MASK_OP means it is an operator of mask
operands and itself don't have mask operand. If the operation depends on
a mask, then the name should be UNARY_MASK_OP_TAMA.
For the WIDEN_TERNARY_OP, This is because of the design problem of
vwmacc pattern (as bellow), maybe it can be unified with wmacc and then
WIDEN_TERNARY_OP is not needed.
(define_insn "@pred_widen_mul_plus<su><mode>"
[(set (match_operand:VWEXTI 0 "register_operand")
(if_then_else:VWEXTI
(unspec:<VM>
[(match_operand:<VM> 1 "vector_mask_operand")
(match_operand 5 "vector_length_operand")
(match_operand 6 "const_int_operand")
(match_operand 7 "const_int_operand")
(match_operand 8 "const_int_operand")
(reg:SI VL_REGNUM)
(reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
(plus:VWEXTI
(mult:VWEXTI
(any_extend:VWEXTI
(match_operand:<V_DOUBLE_TRUNC> 3 "register_operand"))
(any_extend:VWEXTI
(match_operand:<V_DOUBLE_TRUNC> 4 "register_operand")))
(match_operand:VWEXTI 2 "register_operand"))
(match_dup 2)))]
"TARGET_VECTOR"
"vwmacc<u>.vv\t%0,%3,%4%p1"
[(set_attr "type" "viwmuladd")
(set_attr "mode" "<V_DOUBLE_TRUNC>")])
In general we use the NULLARY, UNARY, BINARY, TERNARY prefixes just
to determine the number of sources which doesn't seem really necessary
because a user of e.g. NEG will already know that there only is one
source - he already specified it and currently needs to, redundantly,
say UNARY again.
From the caller's point of view, he does know how many sources, but the
emit_insn method needs to use this flag(NULLARY_OP_P, UNARY_OP_P) to
determine how many sources to add.
Oh, you mean we can get oerands number from insn_data[icode].n_operands?
In this case, we don't really need to distinguish between NULLARY, UNARY
and ect.
If we split off the destination and sources from mask, merge and the rest
we could ditch them altogether.
What about
emit_(non)vlmax_insn (icode, *operands (just dest and sources),
mask, merge, tail/mask policy, frm)
with mask defaulting to NULL and merge defaulting to VUNDEF? So ideally,
and in the easy case, the call would just degenerate to
emit_..._insn (icode, operands).
I realize this will cause some complications on the "other side" but with
the enum in place it should still be doable?
That is to say, when the user needs to emit a COND_NEG, do you need to
call it like this?:
emit_vlmax_insn (pred_neg, ops, mask, vundef(), ta, ma)
Is there too much focus on operands that don't need to be passed in?
Like merge, tail and mask policy operands.
Maybe I don't understand enough here, can you explain it in some more
detail?
--
Best,
Lehua
