Thanks Richard.
Do you suggest we should add a macro like this first:
#ifndef DEF_INTERNAL_COND_FN
#define DEF_INTERNAL_COND_FN(NAME, FLAGS, OPTAB, TYPE) \
DEF_INTERNAL_OPTAB_FN (COND_##NAME, FLAGS, cond_##optab, cond_##TYPE)
DEF_INTERNAL_OPTAB_FN (COND_LEN_##NAME, FLAGS, cond_len_##optab,
cond_len_##TYPE)
#endif
If yes, maybe I should first do this in a single patch first?
juzhe.zh...@rivai.ai
From: Richard Sandiford
Date: 2023-07-25 17:41
To: 钟居哲
CC: rguenther; gcc-patches
Subject: Re: [PATCH] VECT: Support CALL vectorization for COND_LEN_*
钟居哲 <juzhe.zh...@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
