[Bug target/110751] RISC-V: Suport undefined value that allows VSETVL PASS use TA/MA

Fri Jul 21 01:53:51 GMT 2023

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110751

--- Comment #16 from xuli1 at eswincomputing dot com <xuli1 at eswincomputing dot com> ---
(In reply to rguenther@suse.de from comment #12)
> On Thu, 20 Jul 2023, juzhe.zhong at rivai dot ai wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110751
> > 
> > --- Comment #11 from JuzheZhong <juzhe.zhong at rivai dot ai> ---
> > (In reply to rguenther@suse.de from comment #10)
> > > On Thu, 20 Jul 2023, juzhe.zhong at rivai dot ai wrote:
> > > 
> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110751
> > > > 
> > > > --- Comment #9 from JuzheZhong <juzhe.zhong at rivai dot ai> ---
> > > > (In reply to rguenther@suse.de from comment #8)
> > > > > On Thu, 20 Jul 2023, juzhe.zhong at rivai dot ai wrote:
> > > > > 
> > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110751
> > > > > > 
> > > > > > --- Comment #6 from JuzheZhong <juzhe.zhong at rivai dot ai> ---
> > > > > > (In reply to rguenther@suse.de from comment #5)
> > > > > > > On Thu, 20 Jul 2023, kito at gcc dot gnu.org wrote:
> > > > > > > 
> > > > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110751
> > > > > > > > 
> > > > > > > > --- Comment #4 from Kito Cheng <kito at gcc dot gnu.org> ---
> > > > > > > > > OK, so TA is either merge or all-ones.
> > > > > > > > 
> > > > > > > > Yes, your understand is correct, just few more detail is that can be mixing
> > > > > > > > with either merge or all-ones.
> > > > > > > > 
> > > > > > > > e.g.
> > > > > > > > 
> > > > > > > > An 4 x i32 vector with mask 1 0 1 0
> > > > > > > > 
> > > > > > > > Op  =  | a | b | c | d |
> > > > > > > > Mask = | 1 | 0 | 1 | 0 |
> > > > > > > > 
> > > > > > > > the result could be:
> > > > > > > > | a | b | c | d |
> > > > > > > > | a | all-1 | c | d |
> > > > > > > > | a | all-1 | c | all-1 |
> > > > > > > > | a | all-1 | c | d |
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > Not sure how you can use MA at the moment since you specify an existing operand in your target hook.  As far as
> > > > > > > > > I can see there's no value the target hook can provide that matches any
> > > > > > > > of the implementation semantics?
> > > > > > > > 
> > > > > > > > That's the key point - we don't know how to return an undefined value there, we
> > > > > > > > have intrinsic can generate undefined value, but it seems impossible to
> > > > > > > > generate that within the hook.
> > > > > > > 
> > > > > > > Well, neither *A nor *U can be specified currently.  As said for 'merge'
> > > > > > > we would need another operand.  And since 'unspecified' is either merge
> > > > > > > or all-ones we can't express that either.  It's not really 'undefined'
> > > > > > > either.
> > > > > > > 
> > > > > > > Note this also means the proposal to define a .MASK_LOAD as zeroing
> > > > > > > masked elements is not going to work for RISC-V, instead we'd need
> > > > > > > an explicit 'else' value there as well.
> > > > > > > 
> > > > > > > In fact we could follow .MASK_LOAD for .COND_* and simply omit
> > > > > > > the 'else' operand for the case of 'unspecified', no?  GIMPLE would
> > > > > > > be fine omitting it, not sure whether there's precedent for
> > > > > > > optabs with optional operands?
> > > > > > 
> > > > > > For RVV auto-vectorization, we define COND_LEN_* has else value in the
> > > > > > arguments. But the else value is not always the real value we need to
> > > > > > care about, this is the code from vectorizable_operation:
> > > > > > 
> > > > > >           if (reduc_idx >= 0)
> > > > > >             {
> > > > > >               /* Perform the operation on active elements only and take
> > > > > >                  inactive elements from the reduction chain input.  */
> > > > > >               gcc_assert (!vop2);
> > > > > >               vops.quick_push (reduc_idx == 1 ? vop1 : vop0);
> > > > > >             }
> > > > > >           else
> > > > > >             {
> > > > > >               auto else_value = targetm.preferred_else_value
> > > > > >                 (cond_fn, vectype, vops.length () - 1, &vops[1]);
> > > > > >               vops.quick_push (else_value);
> > > > > >             }
> > > > > > 
> > > > > > 
> > > > > > You can see for reduction operations, the else value is the real value we
> > > > > > need to depend on, we should use "TU" (Undisturbed or merge value) in RVV.
> > > > > > Meaning the inactive elements should remain the "old" value that's why we
> > > > > > use "TU".
> > > > > 
> > > > > Sure.  For the above case that's obviously correct.
> > > > > 
> > > > > > However, for single binary operations for example, division, we just only
> > > > > > need to forbid the division operations of the inactive elements in the 
> > > > > > hardware, we don't care the value of the inactive elements value. so in
> > > > > > this case, we want to use "TA". In this case, we want the else value be
> > > > > > a meaningless placeholder in Gimple IR (similar to "undef" or "poison" in
> > > > > > LLVM).
> > > > > > 
> > > > > > Such meaningless placeholder in the argument of Gimple IR, can be beneficail
> > > > > > for RVV for 2 following reasons:
> > > > > > 1. allow us use "TA".
> > > > > > 2. Doesn't consume a register.
> > > > > > 
> > > > > > I am not sure whether we can represent such placeholder in Gimple IR.
> > > > > 
> > > > > As said, just drop the 'else' operand and assign 'unspecified' to its
> > > > > semantics?  Like we do for .LEN_MASK_LOAD where there isn't any
> > > > > 'else' value and I presume you'll use 'TA' as well there?
> > > > 
> > > > 
> > > > Yes, LEN_MASK_LOAD doesn't have else value, then we use "TA".
> > > > 
> > > > LEN_MASK_LOAD always doesn't have else value.
> > > > But COND_LEN_xxx, sometimes has else value (for reduction),
> > > > some times doesn't have else value (for division).
> > > > 
> > > > Could you tell me how to simulate COND_LEN_xxx that doesn't have else value
> > > > like
> > > > LEN_MASK_LOAD in consider COND_LEN_xxx pattens may be used in reduction need
> > > > else value?
> > > 
> > > In your target hook simply return NULL_TREE?  The expander should then
> > > omit the else value and you'd have two define_insn, one without
> > > the else value using 'TA' and one with using 'TU' (where it would
> > > match_operand 0).
> > 
> > 
> > Oh, I was thinking returning NULL_TREE will cause ICE.
> 
> It might need fixups (create the call with one less parameter),
> but I think it should be the easiest way to go.
> 
> > But it worth a try. 
> > 
> > @Li Xu: Could you have a try by followings Richard's suggestion and get
> >         back to us?
> > 
> > Thanks.
> > 
> >

OK.