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

[Richard Sandiford <richard.sandiford@arm.com>]

钟居哲 <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.

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