On Wed, 12 Jan 2022, Richard Sandiford wrote:
> Richard Biener <rguent...@suse.de> writes:
> > On Wed, 12 Jan 2022, Richard Sandiford wrote:
> >
> >> Richard Biener <rguent...@suse.de> writes:
> >> > On Wed, 12 Jan 2022, Andre Vieira (lists) wrote:
> >> >
> >> >> Hi,
> >> >>
> >> >> This a fix for the regression caused by '[vect] Re-analyze all modes for
> >> >> epilogues'. The earlier patch assumed there was always at least one
> >> >> other mode
> >> >> than VOIDmode, but that does not need to be the case.
> >> >> If we are dealing with a target that does not define more modes for
> >> >> 'autovectorize_vector_modes', the behaviour before the patch would be
> >> >> to try
> >> >> to create an epilogue for the same autodetected_vector_mode, which
> >> >> unless the
> >> >> target supported partial vectors would always fail. So as a fix I
> >> >> suggest
> >> >> trying to vectorize the epilogue with the preferred_simd_mode for QI,
> >> >> mimicking autovectorize_vector_mode, which will be skipped if it is not
> >> >> a
> >> >> vector_mode (since that already should indicate partial vectors aren't
> >> >> possible) or if no partial vectors are supported and its pessimistic
> >> >> NUNITS is
> >> >> larger than the main loop's VF.
> >> >>
> >> >> Currently bootstrapping and regression testing, otherwise OK for trunk?
> >> >> Can
> >> >> someone verify this fixes the issue for PR103971 on powerpc?
> >> >
> >> > Why not simply start at mode_i = 0 which means autodetecting the mode
> >> > to use for the epilogue? That appears to be a much simpler solution to
> >> > me, including for targets where there are more than one element in the
> >> > vector.
> >>
> >> VOIDmode doesn't tell us anything about what the autodetected mode
> >> will be, so current short-circuit:
> >>
> >> /* If the target does not support partial vectors we can shorten the
> >> number of modes to analyze for the epilogue as we know we can't pick a
> >> mode that has at least as many NUNITS as the main loop's vectorization
> >> factor, since that would imply the epilogue's vectorization factor
> >> would be at least as high as the main loop's and we would be
> >> vectorizing for more scalar iterations than there would be left. */
> >> if (!supports_partial_vectors
> >> && maybe_ge (GET_MODE_NUNITS (vector_modes[mode_i]), first_vinfo_vf))
> >> {
> >> mode_i++;
> >> if (mode_i == vector_modes.length ())
> >> break;
> >> continue;
> >> }
> >>
> >> wouldn't be effective.
> >
> > Well, before this change we simply did
> >
> > - /* Handle the case that the original loop can use partial
> > - vectorization, but want to only adopt it for the epilogue.
> > - The retry should be in the same mode as original. */
> > - if (LOOP_VINFO_EPIL_USING_PARTIAL_VECTORS_P (first_loop_vinfo))
> > ...
> > - else
> > - {
> > - mode_i = first_loop_next_i;
> > - if (mode_i == vector_modes.length ())
> > - return first_loop_vinfo;
> > - }
> >
> > and thus didn't bother with epilogue vectorization. I think we should
> > then just restore this behavior, not doing epilogue vectorization
> > if vector_modes.length () == 1?
>
> Yeah, but that case didn't need epilogue vectorisation before. This
> series is adding support for unrolling, and targets with a single vector
> size will benefit from epilogues in that case.
But in that case (which we could detect), we could then just use
autodetected_vector_mode? Like if we do before epilogue vect
vector_modes[0] = autodetected_vector_mode;
mode_i = 0;
thus replace VOIDmode with what we detected and then start at 0?
That is, the proposed patch looks very much like a hack to me.
I suppose the VECTOR_MODE_P check should be added to
if (!supports_partial_vectors
&& maybe_ge (GET_MODE_NUNITS (vector_modes[mode_i]),
first_vinfo_vf))
{
mode_i++;
instead.
> Thanks,
> Richard
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
