Re: vectorizer: Avoid an OOB access from vectorization

[Richard Biener <rguenther@suse.de>]

On Tue, 18 Jul 2023, Matthew Malcomson wrote:

> Tamar pointed out it would be good to have a `scan-tree-dump` in the testcase
> just to make sure that when something is currently vectorizing it stays
> vectorizing (and hence that the new code is still likely running).
> 
> Attached patch has that change, also inlined for ease of reply.

The patch is OK with a suitable changelog.

Thanks,
Richard.

> ------------------------------
> > Our checks for whether the vectorization of a given loop would make an
> > out of bounds access miss the case when the vector we load is so large
> > as to span multiple iterations worth of data (while only being there to
> > implement a single iteration).
> > 
> > This patch adds a check for such an access.
> > 
> > Example where this was going wrong (smaller version of testcase added):
> > 
> > ```
> >   extern unsigned short multi_array[5][16][16];
> >   extern void initialise_s(int *);
> >   extern int get_sval();
> > 
> >   void foo() {
> >     int s0 = get_sval();
> >     int s[31];
> >     int i,j;
> >     initialise_s(&s[0]);
> >     s0 = get_sval();
> >     for (j=0; j < 16; j++)
> >       for (i=0; i < 16; i++)
> > 	multi_array[1][j][i]=s[j*2];
> >   }
> > ```
> > 
> > With the above loop we would load the `s[j*2]` integer into a 4 element
> > vector, which reads 3 extra elements than the scalar loop would.
> > `get_group_load_store_type` identifies that the loop requires a scalar
> > epilogue due to gaps.  However we do not identify that the above code
> > requires *two* scalar loops to be peeled due to the fact that each
> > iteration loads an amount of data from the *next* iteration (while not
> > using it).
> > 
> > Bootstrapped and regtested on aarch64-none-linux-gnu.
> > N.b. out of interest we came across this working with Morello.
> > 
> > 
> 
> 
> ###############     Attachment also inlined for ease of reply    ###############
> 
> 
> diff --git a/gcc/testsuite/gcc.dg/vect/vect-multi-peel-gaps.c b/gcc/testsuite/gcc.dg/vect/vect-multi-peel-gaps.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..1aab4c5a14d1e8346d89587bd9544a1516535a45
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vect/vect-multi-peel-gaps.c
> @@ -0,0 +1,61 @@
> +/* For some targets we end up vectorizing the below loop such that the `sp`
> +   single integer is loaded into a 4 integer vector.
> +   While the writes are all safe, without 2 scalar loops being peeled into the
> +   epilogue we would read past the end of the 31 integer array.  This happens
> +   because we load a 4 integer chunk to only use the first integer and
> +   increment by 2 integers at a time, hence the last load needs s[30-33] and
> +   the penultimate load needs s[28-31].
> +   This testcase ensures that we do not crash due to that behaviour.  */
> +/* { dg-require-effective-target mmap } */
> +#include <sys/mman.h>
> +#include <stdio.h>
> +
> +#define MMAP_SIZE 0x20000
> +#define ADDRESS 0x1122000000
> +
> +#define MB_BLOCK_SIZE 16
> +#define VERT_PRED_16 0
> +#define HOR_PRED_16 1
> +#define DC_PRED_16 2
> +int *sptr;
> +extern void intrapred_luma_16x16();
> +unsigned short mprr_2[5][16][16];
> +void initialise_s(int *s) { }
> +int main() {
> +    void *s_mapping;
> +    void *end_s;
> +    s_mapping = mmap ((void *)ADDRESS, MMAP_SIZE, PROT_READ | PROT_WRITE,
> +		      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
> +    if (s_mapping == MAP_FAILED)
> +      {
> +	perror ("mmap");
> +	return 1;
> +      }
> +    end_s = (s_mapping + MMAP_SIZE);
> +    sptr = (int*)(end_s - sizeof(int[31]));
> +    intrapred_luma_16x16(sptr);
> +    return 0;
> +}
> +
> +void intrapred_luma_16x16(int * restrict sp) {
> +    for (int j=0; j < MB_BLOCK_SIZE; j++)
> +      {
> +	mprr_2[VERT_PRED_16][j][0]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][1]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][2]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][3]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][4]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][5]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][6]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][7]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][8]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][9]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][10]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][11]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][12]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][13]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][14]=sp[j*2];
> +	mprr_2[VERT_PRED_16][j][15]=sp[j*2];
> +      }
> +}
> +/* { dg-final { scan-tree-dump "LOOP VECTORIZED" "vect" {target vect_int } } } */
> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> index c08d0ef951fc63adcfffc601917134ddf51ece45..1c8c6784cc7b5f2d327339ff55a5a5ea08835aab 100644
> --- a/gcc/tree-vect-stmts.cc
> +++ b/gcc/tree-vect-stmts.cc
> @@ -2217,7 +2217,9 @@ get_group_load_store_type (vec_info *vinfo, stmt_vec_info stmt_info,
>  	     but the access in the loop doesn't cover the full vector
>  	     we can end up with no gap recorded but still excess
>  	     elements accessed, see PR103116.  Make sure we peel for
> -	     gaps if necessary and sufficient and give up if not.  */
> +	     gaps if necessary and sufficient and give up if not.
> +	     If there is a combination of the access not covering the full vector and
> +	     a gap recorded then we may need to peel twice.  */
>  	  if (loop_vinfo
>  	      && *memory_access_type == VMAT_CONTIGUOUS
>  	      && SLP_TREE_LOAD_PERMUTATION (slp_node).exists ()
> @@ -2233,7 +2235,7 @@ get_group_load_store_type (vec_info *vinfo, stmt_vec_info stmt_info,
>  		     access excess elements.
>  		     ???  Enhancements include peeling multiple iterations
>  		     or using masked loads with a static mask.  */
> -		  || (group_size * cvf) % cnunits + group_size < cnunits)
> +		  || (group_size * cvf) % cnunits + group_size - gap < cnunits)
>  		{
>  		  if (dump_enabled_p ())
>  		    dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> 
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)