[PATCH] slp tree vectorizer: Re-calculate vectorization factor in the case of invalid choices [PR96974]

[Richard Biener]

On Wed, 31 Mar 2021, Stam Markianos-Wright wrote:

> On 29/03/2021 10:20, Richard Biener wrote:
> > On Fri, 26 Mar 2021, Richard Sandiford wrote:
> > 
> >> Richard Biener <rguenther@suse.de> writes:
> >>> On Wed, 24 Mar 2021, Stam Markianos-Wright wrote:
> >>>
> >>>> Hi all,
> >>>>
> >>>> This patch resolves bug:
> >>>>
> >>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96974
> >>>>
> >>>> This is achieved by forcing a re-calculation of *stmt_vectype_out if an
> >>>> incompatible combination of TYPE_VECTOR_SUBPARTS is detected, but with an
> >>>> extra introduced max_nunits ceiling.
> >>>>
> >>>> I am not 100% sure if this is the best way to go about fixing this,
> >>>> because
> >>>> this is my first look at the vectorizer and I lack knowledge of the wider
> >>>> context, so do let me know if you see a better way to do this!
> >>>>
> >>>> I have added the previously ICE-ing reproducer as a new test.
> >>>>
> >>>> This is compiled as "g++ -Ofast -march=armv8.2-a+sve -fdisable-tree-fre4"
> >>>> for
> >>>> GCC11 and "g++ -Ofast -march=armv8.2-a+sve" for GCC10.
> >>>>
> >>>> (the non-fdisable-tree-fre4 version has gone latent on GCC11)
> >>>>
> >>>> Bootstrapped and reg-tested on aarch64-linux-gnu.
> >>>> Also reg-tested on aarch64-none-elf.
> >>>
> >>> I don't think this is going to work well given uses will expect
> >>> a vector type that's consistent here.
> >>>
> >>> I think giving up is for the moment the best choice, thus replacing
> >>> the assert with vectorization failure.
> >>>
> >>> In the end we shouldn't require those nunits vectypes to be
> >>> separately computed - we compute the vector type of the defs
> >>> anyway and in case they're invariant the vectorizable_* function
> >>> either can deal with the type mix or not anyway.
> >>
> >> I agree this area needs simplification, but I think the direction of
> >> travel should be to make the assert valid.  I agree this is probably
> >> the pragmatic fix for GCC 11 and earlier though.
> > 
> > The issue is that we compute a vector type for a use that may differ
> > from what we'd compute for it in the context of its definition (or
> > in the context of another use).  Any such "local" decision is likely
> > flawed and I'd rather simplify further doing the only decision on
> > the definition side - if there's a disconnect between the number
> > of lanes (and thus altering the VF won't help) then we have to give
> > up anyway.
> > 
> > Richard.
> > 
> 
> Thank you both for the further info! Would it be fair to close the initial PR
> regarding the ICE (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96974) and
> then open a second one at a lower priority level to address these further
> improvements?

I have closed the original report.  If there's a testcase where
vectorization is possible and useful but is now rejected because of
the fix then yes, please open a new missed-optimization bugreport.

Richard.