Re: [PATCH] RISC-V: Allow RVV VMS{Compare}(V1, V1) simplify to VMSET

[Jeff Law <jeffreyalaw@gmail.com>]

On 4/29/23 19:40, Kito Cheng wrote:
> Hi Jeff:
> 
> The RTL pattern already models tail element and vector length well,
> so I don't feel the first version of Pan's patch has any problem?
> 
> Input RTL pattern:
> 
> #(insn 10 7 12 2 (set (reg:VNx2BI 134 [ _1 ])
> #        (if_then_else:VNx2BI (unspec:VNx2BI [
> #                    (const_vector:VNx2BI repeat [
> #                            (const_int 1 [0x1])
> #                        ])  # all-1 mask
> #                    (reg:DI 143)  # AVL reg, or vector length
> #                    (const_int 2 [0x2]) # mask policy
> #                    (const_int 0 [0])   # avl type
> #                    (reg:SI 66 vl)
> #                    (reg:SI 67 vtype)
> #                ] UNSPEC_VPREDICATE)
> #            (geu:VNx2BI (reg/v:VNx2QI 137 [ v1 ])
> #                (reg/v:VNx2QI 137 [ v1 ]))
> #            (unspec:VNx2BI [
> #                    (reg:SI 0 zero)
> #                ] UNSPEC_VUNDEF))) # maskoff and tail operand
> #     (expr_list:REG_DEAD (reg:DI 143)
> #        (expr_list:REG_DEAD (reg/v:VNx2QI 137 [ v1 ])
> #            (nil))))
> 
> And the split pattern, only did on tail/maskoff element with undefined value:
> 
> (define_split
>   [(set (match_operand:VB      0 "register_operand")
>         (if_then_else:VB
>           (unspec:VB
>             [(match_operand:VB 1 "vector_all_trues_mask_operand")
>              (match_operand    4 "vector_length_operand")
>              (match_operand    5 "const_int_operand")
>              (match_operand    6 "const_int_operand")
>              (reg:SI VL_REGNUM)
>              (reg:SI VTYPE_REGNUM)] UNSPEC_VPREDICATE)
>           (match_operand:VB    3 "vector_move_operand")
>           (match_operand:VB    2 "vector_undef_operand")))] # maskoff
> and tail operand, only match undef value
> 
> Then it turns into vmset, and also discard mask policy operand (since
> maskoff is undef means don't care IMO):
> 
> (insn 10 7 12 2 (set (reg:VNx2BI 134 [ _1 ])
>         (if_then_else:VNx2BI (unspec:VNx2BI [
>                     (const_vector:VNx2BI repeat [
>                             (const_int 1 [0x1])
>                         ])  # all-1 mask
>                     (reg:DI 143) # AVL reg, or vector length
>                     (const_int 2 [0x2]) # mask policy
>                     (reg:SI 66 vl)
>                     (reg:SI 67 vtype)
>                 ] UNSPEC_VPREDICATE)
>             (const_vector:VNx2BI repeat [
>                     (const_int 1 [0x1])
>                 ])    # all-1
>             (unspec:VNx2BI [
>                     (reg:SI 0 zero)
>                 ] UNSPEC_VUNDEF))) # still vundef
>      (expr_list:REG_DEAD (reg:DI 143)
>         (nil)))
Right.  My concern is that when we call relational_result it's going to 
return -1 (as a vector of bools) which bubbles up through the call 
chain.   If that doesn't match the actual register state after the 
instruction (irrespective of the tail policy), then we have the 
potential to generate incorrect code.

For example, if there's a subsequent instruction that tried to set a 
vector register to -1, it could just copy from the destination of the 
vmset to the new target.  But if the vmset didn't set all the bits to 1, 
then the code is wrong.

With all the UNSPECs in place, this may not be a problem in practice. 
Unsure.  I'm willing to defer to you on this Kito.

Jeff