Hi Tamar,
Glad to see this series :)
I'm still trying to wrap my head around the changes as well as the
status before, and what we need for RVV in order to make proper use of
it.
Also, I'm getting the impression that this is the first time we could
use two separate else values (one for the tail, one for the inactive
elements). But let's not go there now, I don't want to make people
detest RVV even more :)
One main point I'd still like to raise, though. If that has been
discussed already, feel free to ignore:
Right now both the mask as well as the len path mask the comparison
(where the m & cmp comes from that you want to optimize) and not the
following "reduction" (= COUNT_ACTIVE) that feeds the branch. For RVV
that's surely not the best solution and I'd rather have a plain vector
comparison, implicitly length masked, as input for
LEN_COUNT_ACTIVE/LEN_MASK_INDEX_FIRST so that the m & cmp never even
gets emitted. For RVV that approach should always better, for SVE I'm
not so sure. I could imagine that the predicate handling for ncopies >
1 gets more expensive with that suggestion.
Would masking COUNT_ACTIVE work for SVE, at least for the ncopies = 1
case or am I missing something fundamental? Or maybe it just doesn't
scale beyond ncopies = 1? But wouldn't some of the extra work we need
to do (like the TER and duplicate use) in this series vanish if that
were a viable approach?
One way or another, I was just curious and, of course, I'm not asking
you to change the RVV code path. I'll do that myself it there's still
time during stage 1.
So to my understanding, right now with trapping math excluded, we don't
need a masked vec_cmp for RVV because we could just compare without
length, ignoring tail elements, then popcount the result and branch
dependent on it. With trapping math the cond_len variant is needed but
won't be optimized anyway.
Apart from that, I have mostly bikeshedding to contribute, with the RVV
usage in mind.
> +@cindex @code{IFN_COUNT_ACTIVE} internal function
> +@item @samp{IFN_COUNT_ACTIVE}
> +The @code{IFN_COUNT_ACTIVE} internal function directly maps to the
> +@code{count_active} instruction pattern. It returns the number of active
> +elements in its argument.
> +
> +@cindex @code{IFN_COND_COUNT_ACTIVE} internal function
> +@item @samp{IFN_COND_COUNT_ACTIVE}
> +The @code{IFN_COND_COUNT_ACTIVE} internal function directly maps to the
> +@code{cond_count_active} instruction pattern. It returns the number of
> +active elements in its second argument under the mask in its first
> +argument.
> +
> +@cindex @code{IFN_COND_LEN_COUNT_ACTIVE} internal function
> +@item @samp{IFN_COND_LEN_COUNT_ACTIVE}
> +The @code{IFN_COND_LEN_COUNT_ACTIVE} internal function directly maps to the
> +@code{cond_len_count_active} instruction pattern. It is like
> +@code{IFN_COND_COUNT_ACTIVE}, but also ignores elements whose index is
> +greater than or equal to the sum of the length and bias arguments.
> +
> +@cindex @code{IFN_INDEX_FIRST_ACTIVE} internal function
> +@item @samp{IFN_INDEX_FIRST_ACTIVE}
> +The @code{IFN_INDEX_FIRST_ACTIVE} internal function directly maps to the
> +@code{index_first_active} instruction pattern. It returns the index of the
> +first active element in its argument. The result is undefined if the
> +predicate has no active elements.
Could we define the result as -1? RVV's vfirst returns -1 if no bits
are set and that would help to rely on it.
I know I mentioned naming before, sorry, but I got confused about
"active" again. The functions here refer to the set bits in a
vector/mask as active while there can be fewer active lanes than set
bits. So arguably, we should rather call them INDEX_FIRST_SET etc. but
I'm not going to ask you change everything unless there's consensus.
The one I mentioned before, EXTRACT_FIRST_ACTIVE is still ok I guess,
because it actually extracts an active lane. But the IFNs operating on
predicates don't. I just realized RVV's insns already follow the
set-naming (like vmsbf vector mask set before first). Believe it or
not, that's not where I'm coming from :)
Usually we add len ifns (as well as mask ifns) to internal_fn_len_index
and internal_fn_mask_index but as we can't re-use
add_mask_else_and_len_args due to optab operand order, it won't help
anyway. And I really hope we can untangle all of this eventually when
we have an RTL vector predicate.
> +
> +@cindex @code{IFN_COND_INDEX_FIRST_ACTIVE} internal function
> +@item @samp{IFN_COND_INDEX_FIRST_ACTIVE}
> +The @code{IFN_COND_INDEX_FIRST_ACTIVE} internal function directly maps to the
> +@code{cond_index_first_active} instruction pattern. It returns the index of
> +the first active element in its second argument under the mask in its first
> +argument. The result is undefined if the selected predicate has no active
> +elements.
Maybe a short note here that these IFNs take a predicate/mask type as
input?
--
Regards
Robin
