Re: [PATCH PR95570] vect: ICE: Segmentation fault in vect_loop_versioning
"Yangfei (Felix)" writes: > +2020-06-12 Felix Yang > + > + PR tree-optimization/95570 > + * tree-vect-data-refs.c (vect_relevant_for_alignment_p): New function. > + (vect_verify_datarefs_alignment): Call it to filter out data > references > + in the loop whose alignment is irrelevant. > + (vect_get_peeling_costs_all_drs): Likewise. > + (vect_peeling_supportable): Likewise. > + (vect_enhance_data_refs_alignment): Likewise. > > gcc/testsuite/ > > +2020-06-12 Felix Yang > + > + PR tree-optimization/95570 > + * gcc.dg/vect/pr95570.c: New test. Pushed to master, thanks. Richard
RE: [PATCH PR95570] vect: ICE: Segmentation fault in vect_loop_versioning
Hi,
> -Original Message-
> From: Richard Sandiford [mailto:richard.sandif...@arm.com]
> Sent: Friday, June 12, 2020 2:29 AM
> To: Yangfei (Felix)
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH PR95570] vect: ICE: Segmentation fault in
> vect_loop_versioning
>
> "Yangfei (Felix)" writes:
> > From 30a0196b0afd45bae9291cfab3fee4ad6b90cbcb Mon Sep 17 00:00:00
> 2001
> > From: Fei Yang
> > Date: Thu, 11 Jun 2020 19:33:22 +0800
> > Subject: [PATCH] vect: Fix an ICE in vect_loop_versioning [PR95570]
> >
> > In the test case for PR95570, the only data reference in the loop is a
> > gather-statter access. Scalar evolution analysis for this data
> > reference failed, so DR_STEP is NULL_TREE. This leads to the segmentation
> fault.
> > We should filter out scatter-gather access in
> vect_enhance_data_refs_alignment.
>
> Looks good, just a couple of very minor details…
>
> > 2020-06-11 Felix Yang
> >
> > gcc/
> > PR tree-optimization/95570
> > * tree-vect-data-refs.c (vect_relevant_for_alignment_p): New
> function.
> > (vect_verify_datarefs_alignment): Call it to filter out data
> > references
> > in the loop whose alignment is irrelevant.
> > (vect_get_peeling_costs_all_drs): Likewise.
> > (vect_peeling_supportable): Likewise.
> > (vect_enhance_data_refs_alignment): Likewise.
> >
> > gcc/testsuite/
> >
> > PR tree-optimization/95570
> > * gcc.dg/vect/pr95570.c: New test.
> > ---
> > gcc/testsuite/gcc.dg/vect/pr95570.c | 11
> > gcc/tree-vect-data-refs.c | 83 -
> > 2 files changed, 45 insertions(+), 49 deletions(-) create mode
> > 100644 gcc/testsuite/gcc.dg/vect/pr95570.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/vect/pr95570.c
> > b/gcc/testsuite/gcc.dg/vect/pr95570.c
> > new file mode 100644
> > index 000..b9362614004
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/vect/pr95570.c
> > @@ -0,0 +1,11 @@
> > +/* { dg-do compile } */
> > +/* { dg-additional-options "-march=armv8.2-a+sve
> > +-msve-vector-bits=256 -mstrict-align -fwrapv" { target aarch64*-*-* }
> > +} */
> > +
> > +int x[8][32];
> > +
> > +void
> > +foo (int start)
> > +{
> > + for (int i = start; i < start + 16; i++)
> > +x[start][i] = i;
> > +}
> > diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c
> > index 39d5a1b554c..98f6fb76ff9 100644
> > --- a/gcc/tree-vect-data-refs.c
> > +++ b/gcc/tree-vect-data-refs.c
> > @@ -1129,6 +1129,35 @@ vect_update_misalignment_for_peel
> (dr_vec_info *dr_info,
> >SET_DR_MISALIGNMENT (dr_info, DR_MISALIGNMENT_UNKNOWN); }
> >
> > +/* Return TRUE if alignment is relevant for DR_INFO. */
>
> Just “Return true …“ for new code. TRUE is a hold-over from C days.
OK.
> > +static bool
> > +vect_relevant_for_alignment_p (dr_vec_info *dr_info) {
> > + stmt_vec_info stmt_info = dr_info->stmt;
> > +
> > + if (!STMT_VINFO_RELEVANT_P (stmt_info))
> > +return false;
> > +
> > + /* For interleaving, only the alignment of the first access
> > + matters. */ if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
> > + && DR_GROUP_FIRST_ELEMENT (stmt_info) != stmt_info)
> > +return false;
> > +
> > + /* For scatter-gather or invariant accesses, alignment is irrelevant
> > + for them. */
>
> Maybe:
>
> /* Scatter-gather and invariant accesses continue to address individual
> scalars, so vector-level alignment is irrelevant. */
Much better : - ) Updated accordingly.
Also bootstrapped and tested on x86_64-linux-gnu.
Thanks,
Felix
gcc/
+2020-06-12 Felix Yang
+
+ PR tree-optimization/95570
+ * tree-vect-data-refs.c (vect_relevant_for_alignment_p): New function.
+ (vect_verify_datarefs_alignment): Call it to filter out data references
+ in the loop whose alignment is irrelevant.
+ (vect_get_peeling_costs_all_drs): Likewise.
+ (vect_peeling_supportable): Likewise.
+ (vect_enhance_data_refs_alignment): Likewise.
gcc/testsuite/
+2020-06-12 Felix Yang
+
+ PR tree-optimization/95570
+ * gcc.dg/vect/pr95570.c: New test.
pr95570-v3.diff
Description: pr95570-v3.diff
Re: [PATCH PR95570] vect: ICE: Segmentation fault in vect_loop_versioning
"Yangfei (Felix)" writes:
> From 30a0196b0afd45bae9291cfab3fee4ad6b90cbcb Mon Sep 17 00:00:00 2001
> From: Fei Yang
> Date: Thu, 11 Jun 2020 19:33:22 +0800
> Subject: [PATCH] vect: Fix an ICE in vect_loop_versioning [PR95570]
>
> In the test case for PR95570, the only data reference in the loop is a
> gather-statter access. Scalar evolution analysis for this data reference
> failed, so DR_STEP is NULL_TREE. This leads to the segmentation fault.
> We should filter out scatter-gather access in
> vect_enhance_data_refs_alignment.
Looks good, just a couple of very minor details…
> 2020-06-11 Felix Yang
>
> gcc/
> PR tree-optimization/95570
> * tree-vect-data-refs.c (vect_relevant_for_alignment_p): New function.
> (vect_verify_datarefs_alignment): Call it to filter out data
> references
> in the loop whose alignment is irrelevant.
> (vect_get_peeling_costs_all_drs): Likewise.
> (vect_peeling_supportable): Likewise.
> (vect_enhance_data_refs_alignment): Likewise.
>
> gcc/testsuite/
>
> PR tree-optimization/95570
> * gcc.dg/vect/pr95570.c: New test.
> ---
> gcc/testsuite/gcc.dg/vect/pr95570.c | 11
> gcc/tree-vect-data-refs.c | 83 -
> 2 files changed, 45 insertions(+), 49 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/vect/pr95570.c
>
> diff --git a/gcc/testsuite/gcc.dg/vect/pr95570.c
> b/gcc/testsuite/gcc.dg/vect/pr95570.c
> new file mode 100644
> index 000..b9362614004
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vect/pr95570.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-march=armv8.2-a+sve -msve-vector-bits=256
> -mstrict-align -fwrapv" { target aarch64*-*-* } } */
> +
> +int x[8][32];
> +
> +void
> +foo (int start)
> +{
> + for (int i = start; i < start + 16; i++)
> +x[start][i] = i;
> +}
> diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c
> index 39d5a1b554c..98f6fb76ff9 100644
> --- a/gcc/tree-vect-data-refs.c
> +++ b/gcc/tree-vect-data-refs.c
> @@ -1129,6 +1129,35 @@ vect_update_misalignment_for_peel (dr_vec_info
> *dr_info,
>SET_DR_MISALIGNMENT (dr_info, DR_MISALIGNMENT_UNKNOWN);
> }
>
> +/* Return TRUE if alignment is relevant for DR_INFO. */
Just “Return true …“ for new code. TRUE is a hold-over from C days.
> +static bool
> +vect_relevant_for_alignment_p (dr_vec_info *dr_info)
> +{
> + stmt_vec_info stmt_info = dr_info->stmt;
> +
> + if (!STMT_VINFO_RELEVANT_P (stmt_info))
> +return false;
> +
> + /* For interleaving, only the alignment of the first access matters. */
> + if (STMT_VINFO_GROUPED_ACCESS (stmt_info)
> + && DR_GROUP_FIRST_ELEMENT (stmt_info) != stmt_info)
> +return false;
> +
> + /* For scatter-gather or invariant accesses, alignment is irrelevant
> + for them. */
Maybe:
/* Scatter-gather and invariant accesses continue to address individual
scalars, so vector-level alignment is irrelevant. */
Thanks,
Richard
RE: [PATCH PR95570] vect: ICE: Segmentation fault in vect_loop_versioning
Hi, > -Original Message- > From: Richard Sandiford [mailto:richard.sandif...@arm.com] > Sent: Thursday, June 11, 2020 12:23 AM > To: Yangfei (Felix) > Cc: gcc-patches@gcc.gnu.org > Subject: Re: [PATCH PR95570] vect: ICE: Segmentation fault in > vect_loop_versioning > > "Yangfei (Felix)" writes: > > Hi, > > > > PR: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95570 > > > > Here, we are doing loop versioning for alignment. The only dr here is a > gather-statter operation: x[start][i]. > > Scalar evolution analysis for this dr failed, so DR_STEP is NULL_TREE, which > leads to the segfault. > > But scatter-gather operation should be filtered out in > vect_enhance_data_refs_alignment. > > There are similar issues in vect_verify_datarefs_alignment, > vect_get_peeling_costs_all_drs and vect_peeling_supportable. > > Proposed patch adds back the necessary tests. Bootstrapped and tested > on aarch64-linux-gnu & x86_64-linux-gnu. > > > > Test coverage: > > Existing tests [1] and newly added test ensures coverage for all the changes > except for the changes in vect_peeling_supportable. > > Currently I don't have a test to cover the changes in > vect_peeling_supportable. Should we keep them? > > Rather than add several instances of the new test, I think it would make > sense to split the (hopefully) correct conditions in > vect_enhance_data_refs_alignment out into a subroutine and use it in the > other sites. Doing that for vect_peeling_supportable would then be > justifiable as a clean-up. OK. > How about something like vect_relevant_for_alignment_p as the name of > the subroutine? Nice name. Does the v2 patch look better? Bootstrapped and tested on aarch64-linux-gnu. Newly added test fail without the fix and pass otherwise. gcc/ +2020-06-11 Felix Yang + + PR tree-optimization/95570 + * tree-vect-data-refs.c (vect_relevant_for_alignment_p): New function. + (vect_verify_datarefs_alignment): Call it to filter out data references + in the loop whose alignment is irrelevant. + (vect_get_peeling_costs_all_drs): Likewise. + (vect_peeling_supportable): Likewise. + (vect_enhance_data_refs_alignment): Likewise. gcc/testsuite/ +2020-06-11 Felix Yang + + PR tree-optimization/95570 + * gcc.dg/vect/pr95570.c: New test. Thanks, Felix pr95570-v2.diff Description: pr95570-v2.diff
Re: [PATCH PR95570] vect: ICE: Segmentation fault in vect_loop_versioning
"Yangfei (Felix)" writes: > Hi, > > PR: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95570 > > Here, we are doing loop versioning for alignment. The only dr here is a > gather-statter operation: x[start][i]. > Scalar evolution analysis for this dr failed, so DR_STEP is NULL_TREE, which > leads to the segfault. > But scatter-gather operation should be filtered out in > vect_enhance_data_refs_alignment. > There are similar issues in vect_verify_datarefs_alignment, > vect_get_peeling_costs_all_drs and vect_peeling_supportable. > Proposed patch adds back the necessary tests. Bootstrapped and tested on > aarch64-linux-gnu & x86_64-linux-gnu. > > Test coverage: > Existing tests [1] and newly added test ensures coverage for all the changes > except for the changes in vect_peeling_supportable. > Currently I don't have a test to cover the changes in > vect_peeling_supportable. Should we keep them? Rather than add several instances of the new test, I think it would make sense to split the (hopefully) correct conditions in vect_enhance_data_refs_alignment out into a subroutine and use it in the other sites. Doing that for vect_peeling_supportable would then be justifiable as a clean-up. How about something like vect_relevant_for_alignment_p as the name of the subroutine? Thanks, Richard
