Re: [PATCH] Fixing PR59006 and PR58921 by delaying loop invariant hoisting in vectorizer.

[Richard Biener <rguenther@suse.de>]

On Wed, 27 Nov 2013, Jakub Jelinek wrote:

> On Wed, Nov 27, 2013 at 10:53:56AM +0100, Richard Biener wrote:
> > Hmm.  I'm still thinking that we should handle this during the regular
> > transform step.
> 
> I wonder if it can't be done instead just in vectorizable_load,
> if LOOP_REQUIRES_VERSIONING_FOR_ALIAS (loop_vinfo) and the load is
> invariant, just emit the (broadcasted) load not inside of the loop, but on
> the loop preheader edge.

It is safe even for !LOOP_REQUIRES_VERSIONING_FOR_ALIAS.  It's just
a missed optimization I even noted when originally implementing
support for invariant loads ...

> > *** gcc/tree-vect-data-refs.c	(revision 205435)
> > --- gcc/tree-vect-data-refs.c	(working copy)
> > *************** again:
> > *** 3668,3673 ****
> > --- 3668,3682 ----
> >   	    }
> >   	  STMT_VINFO_STRIDE_LOAD_P (stmt_info) = true;
> >   	}
> > +       else if (loop_vinfo
> > + 	       && integer_zerop (DR_STEP (dr)))
> > + 	{
> > + 	  /* All loads from a non-varying address will be disambiguated
> > + 	     by data-ref analysis or via a runtime alias check and thus
> > + 	     they will become invariant.  Force them to be vectorized
> > + 	     as external.  */
> > + 	  STMT_VINFO_DEF_TYPE (stmt_info) = vect_external_def;
> > + 	}
> 
> I think this is unsafe for simd loops.
> I'd say:
> int a[1024], b[1024];
> 
> int foo (void)
> {
>   int i;
>   #pragma omp simd safelen(8)
>   for (i = 0; i < 1024; i++)
>     {
>       a[i] = i;
>       b[i] = a[0];
>     }
> }
> 
> is valid testcase, the loop behaves the same if you execute it
> sequentially, or vectorize using SIMD (hardware or emulated) instructions
> with vectorization factor of 2, 4 or 8, as long as you do all memory
> operations (either using scalar insns or simd instructions) in the order
> they were written, which I believe the vectorizer right now handles
> correctly, but the hoisting this patch wants to perform is not fine,
> unless data ref analysis would prove that it can't alias.  For non-simd
> loops we of course perform that data ref analysis and either version for
> alias, or prove that the drs can't alias, but for simd loops we take as
> given that the loop is safe to be vectorized.  It is, but not for hoisting.

Ick.  I hate this behind-the-back stuff - so safelen doesn't mean
that a[i] and a[0] do not alias.  Note that this will break with
SLP stuff at least as that will re-order reads/writes.  Not sure
how safelen applies to SLP though.  That is

    a[i] = i;
    b[i] = a[0];
    a[i+1] = i+1;
    b[i+1] = a[1];

will eventually end up re-ordering reads/writes in non-obvious
ways.

> So, the above say with emulated SIMD is safe to be executed as:
>   for (i = 0; i < 1024; i += 8)
>     {
>       int tmp;
>       for (tmp = i; tmp < i + 8; tmp++)
> 	a[tmp] = tmp;
>       for (tmp = i; tmp < i + 8; tmp++)
> 	b[tmp] = a[0];
>     }
> but not as:
>   int tempa[8], tmp;
>   /* Hoisted HW or emulated load + splat.  */
>   for (tmp = 0; tmp < 8; tmp++)
>     tempa[tmp] = a[0];
>   for (i = 0; i < 1024; i += 8)
>     {
>       for (tmp = i; tmp < i + 8; tmp++)
> 	a[tmp] = tmp;
>       for (tmp = i; tmp < i + 8; tmp++)
> 	b[tmp] = tempa[tmp];
>     }
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer