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

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

pushed v1 to trunk

On Fri, May 5, 2023 at 8:46 PM Li, Pan2 via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Ok, sounds good. Thank you!
>
> Pan
>
> -----Original Message-----
> From: Kito Cheng <kito.cheng@sifive.com>
> Sent: Friday, May 5, 2023 8:37 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; Wang, Yanzhang <yanzhang.wang@intel.com>
> Subject: Re: [PATCH] RISC-V: Allow RVV VMS{Compare}(V1, V1) simplify to VMSET
>
> 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