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

[Kito Cheng <kito.cheng@sifive.com>]

I will take V1 and commit to trunk after my local test is done :)

On Fri, May 5, 2023 at 8:30 PM Li, Pan2 <pan2.li@intel.com> wrote:
>
> Hi kito,
>
> Could you please help to share any suggestion about the PATCH? Comparing the V1 and V2.
>
> Pan
>
>
> -----Original Message-----
> From: Li, Pan2
> Sent: Wednesday, May 3, 2023 7:18 PM
> To: Jeff Law <jeffreyalaw@gmail.com>; Kito Cheng <kito.cheng@sifive.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>; Andrew Waterman <andrew@sifive.com>
> Subject: RE: [PATCH] RISC-V: Allow RVV VMS{Compare}(V1, V1) simplify to VMSET
>
> Thanks all for comments, will work with kito to make it happen.
>
> Pan
>
> -----Original Message-----
> From: Jeff Law <jeffreyalaw@gmail.com>
> Sent: Wednesday, May 3, 2023 12:28 AM
> To: Kito Cheng <kito.cheng@sifive.com>
> Cc: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>; Andrew Waterman <andrew@sifive.com>
> Subject: Re: [PATCH] RISC-V: Allow RVV VMS{Compare}(V1, V1) simplify to VMSET
>
>
>
> 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