Re: [PATCH] VECT: Support CALL vectorization for COND_LEN_*

[Richard Biener <rguenther@suse.de>]

On Tue, 25 Jul 2023, Richard Sandiford wrote:

> ??? <juzhe.zhong@rivai.ai> writes:
> > Hi, Richi. Thank you so much for review.
> >
> >>> This function doesn't seem to care about conditional vectorization
> >>> support, so why are you changing it?
> >
> > I debug and analyze the code here:
> >
> > Breakpoint 1, vectorizable_call (vinfo=0x3d358d0, stmt_info=0x3dcc820, gsi=0x0, vec_stmt=0x0, slp_node=0x0, cost_vec=0x7fffffffc668) at ../../../riscv-gcc/gcc/tree-vect-stmts.cc:3485
> > 3485        ifn = vectorizable_internal_function (cfn, callee, vectype_out,
> > (gdb) p cfn
> > $1 = CFN_COND_ADD
> > (gdb) call print_gimple_stmt(stdout,stmt,0,0)
> > _9 = .COND_ADD (_27, _6, _8, _6);
> >
> > When target like RISC-V didnt' support COND_* (we only support COND_LEN_*, no support COND_*),
> > ifn will be IFN_LAST.
> > Also, the following code doesn't modify ifn until
> > "internal_fn cond_fn = get_conditional_internal_fn (ifn);"
> > When ifn == IFN_LAST, cond_fn will IFN_LAST too.
> >
> > So, I extend "vectorizable_internal_function" to return COND_LEN_*.
> >
> > Am I going into wrong direction on debug && analysis.
> 
> The main difference between IFN_COND_ADD and IFN_COND_LEN_ADD is that
> IFN_COND_ADD makes logical sense for scalars, whereas IFN_COND_LEN_ADD
> only makes sense for vectors.  So I think if-conversion has to create
> IFN_COND_ADD rather than IFN_COND_LEN_ADD even for targets that only
> support IFN_COND_LEN_ADD.  (Which is what the patch does.)
> 
> IFN_COND_LEN_ADD is really a generalisation of IFN_COND_ADD.  In a
> sense, every target that supports IFN_COND_LEN_ADD also supports
> IFN_COND_ADD.  All we need is two extra arguments.  And the patch
> does seem to take that approach (by adding dummy length arguments).
> 
> So I think there are at least four ways we could go:
> 
> (1) RVV implements cond_* optabs as expanders.  RVV therefore supports
>     both IFN_COND_ADD and IFN_COND_LEN_ADD.  No dummy length arguments
>     are needed at the gimple level.
> 
> (2) Target-independent code automatically provides cond_* optabs
>     based on cond_len_* optabs.  Otherwise the same as (1).
> 
> (3) A gimple pass lowers IFN_COND_ADD to IFN_COND_LEN_ADD on targets
>     that need it.  Could be vec lowering or isel.
> 
> (4) The vectoriser maintains the distinction between IFN_COND_ADD and
>     IFN_COND_LEN_ADD from the outset.  It adds dummy length arguments
>     where necessary.
> 
> The patch is going for (4).  But I think we should at least consider
> whether any of the other alternatives make sense.

I think only (1) and (4) make sense.  The vectorizer would need to
check for IFN_COND_LEN_ADD or IFN_COND_ADD support anyway even
when only emitting IFN_COND_ADD.  Since we've already gone half-way
then we can as well do (4).  That also possibly simplifies N targets
as opposed to just the vectorizer.  So my preference would be (4),
but I'm also fine with (1).

Richard.

> If we do go for (4), I think the get_conditional_len_internal_fn
> is in the wrong place.  It should be applied:
> 
>   internal_fn ifn;
>   if (internal_fn_p (cfn))
>     ifn = as_internal_fn (cfn);
>   else
>     ifn = associated_internal_fn (fndecl);
>   if (ifn != IFN_LAST && direct_internal_fn_p (ifn))
>     {
>       // After this point
> 
> I don't think we should assume that every IFN_COND_* has an associated
> tree code.  It should be possible to create IFN_COND_*s from unconditional
> internal functions too.  So rather than use:
> 
>       tree_code code = conditional_internal_fn_code (ifn);
>       len_ifn = get_conditional_len_internal_fn (code);
> 
> I think we should have an internal-fn helper that returns IFN_COND_LEN_*
> for a given IFN_COND_*.  It could handle IFN_MASK_LOAD -> IFN_MASK_LEN_LOAD
> etc. too.
> 
> It would help if we combined the COND_* and COND_LEN_* definitions.
> For example:
> 
> DEF_INTERNAL_OPTAB_FN (COND_ADD, ECF_CONST, cond_add, cond_binary)
> DEF_INTERNAL_OPTAB_FN (COND_LEN_ADD, ECF_CONST, cond_len_add, cond_len_binary)
> 
> could become:
> 
> DEF_INTERNAL_COND_FN (ADD, ECF_CONST, add, binary)
> 
> that expands to the COND_ADD and COND_LEN_ADD definitions.  This would
> help to keep the definitions in sync and would make it eaiser to do a
> mapping between COND_* and COND_LEN_*.
> 
> Thanks,
> Richard
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)