| Message ID | f33aa0cf786cf49250889463947b62632cf3f205.1694657494.git.linkw@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | vect: Move costing next to the transform for vect store | expand |
On Thu, Sep 14, 2023 at 5:12 AM Kewen Lin <linkw@linux.ibm.com> wrote: > > When making/testing patches to move costing next to the > transform code for vectorizable_store, some ICEs got > exposed when I further refined the costing handlings on > VMAT_ELEMENTWISE. The apparent cause is triggering the > assertion in rs6000 specific function for costing > rs6000_builtin_vectorization_cost: > > if (TARGET_ALTIVEC) > /* Misaligned stores are not supported. */ > gcc_unreachable (); > > I used vect_get_store_cost instead of the original way by > record_stmt_cost with scalar_store for costing, that is to > use one unaligned_store instead, it matches what we use in > transforming, it's a vector store as below: > > else if (group_size >= const_nunits > && group_size % const_nunits == 0) > { > nstores = 1; > lnel = const_nunits; > ltype = vectype; > lvectype = vectype; > } > > So IMHO it's more consistent with vector store instead of > scalar store, with the given compilation option > -mno-allow-movmisalign, the misaligned vector store is > unexpected to be used in vectorizer, but why it's still > adopted? In the current implementation of function > get_group_load_store_type, we always set alignment support > scheme as dr_unaligned_supported for VMAT_ELEMENTWISE, it > is true if we always adopt scalar stores, but as the above > code shows, we could use vector stores for some cases, so > we should use the correct alignment support scheme for it. > > This patch is to ensure the vector store is supported by > further checking with vect_supportable_dr_alignment. The > ICEs got exposed with patches moving costing next to the > transform but they haven't been landed, the test coverage > would be there once they get landed. The affected test > cases are: > - gcc.dg/vect/slp-45.c > - gcc.dg/vect/vect-alias-check-{10,11,12}.c > > btw, I tried to make some correctness test case, but I > realized that -mno-allow-movmisalign is mainly for noting > movmisalign optab and it doesn't guard for the actual hw > vector memory access insns, so I failed to make it unless > I also altered some conditions for them as it. OK. > gcc/ChangeLog: > > * tree-vect-stmts.cc (vectorizable_store): Ensure the generated > vector store for some case of VMAT_ELEMENTWISE is supported. > --- > gcc/tree-vect-stmts.cc | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > index cd7c1090d88..a5caaf0bca2 100644 > --- a/gcc/tree-vect-stmts.cc > +++ b/gcc/tree-vect-stmts.cc > @@ -8558,10 +8558,18 @@ vectorizable_store (vec_info *vinfo, > else if (group_size >= const_nunits > && group_size % const_nunits == 0) > { > - nstores = 1; > - lnel = const_nunits; > - ltype = vectype; > - lvectype = vectype; > + int mis_align = dr_misalignment (first_dr_info, vectype); > + dr_alignment_support dr_align > + = vect_supportable_dr_alignment (vinfo, dr_info, vectype, > + mis_align); > + if (dr_align == dr_aligned > + || dr_align == dr_unaligned_supported) > + { > + nstores = 1; > + lnel = const_nunits; > + ltype = vectype; > + lvectype = vectype; > + } > } > ltype = build_aligned_type (ltype, TYPE_ALIGN (elem_type)); > ncopies = SLP_TREE_NUMBER_OF_VEC_STMTS (slp_node); > -- > 2.31.1 >
diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc index cd7c1090d88..a5caaf0bca2 100644 --- a/gcc/tree-vect-stmts.cc +++ b/gcc/tree-vect-stmts.cc @@ -8558,10 +8558,18 @@ vectorizable_store (vec_info *vinfo, else if (group_size >= const_nunits && group_size % const_nunits == 0) { - nstores = 1; - lnel = const_nunits; - ltype = vectype; - lvectype = vectype; + int mis_align = dr_misalignment (first_dr_info, vectype); + dr_alignment_support dr_align + = vect_supportable_dr_alignment (vinfo, dr_info, vectype, + mis_align); + if (dr_align == dr_aligned + || dr_align == dr_unaligned_supported) + { + nstores = 1; + lnel = const_nunits; + ltype = vectype; + lvectype = vectype; + } } ltype = build_aligned_type (ltype, TYPE_ALIGN (elem_type)); ncopies = SLP_TREE_NUMBER_OF_VEC_STMTS (slp_node);
When making/testing patches to move costing next to the transform code for vectorizable_store, some ICEs got exposed when I further refined the costing handlings on VMAT_ELEMENTWISE. The apparent cause is triggering the assertion in rs6000 specific function for costing rs6000_builtin_vectorization_cost: if (TARGET_ALTIVEC) /* Misaligned stores are not supported. */ gcc_unreachable (); I used vect_get_store_cost instead of the original way by record_stmt_cost with scalar_store for costing, that is to use one unaligned_store instead, it matches what we use in transforming, it's a vector store as below: else if (group_size >= const_nunits && group_size % const_nunits == 0) { nstores = 1; lnel = const_nunits; ltype = vectype; lvectype = vectype; } So IMHO it's more consistent with vector store instead of scalar store, with the given compilation option -mno-allow-movmisalign, the misaligned vector store is unexpected to be used in vectorizer, but why it's still adopted? In the current implementation of function get_group_load_store_type, we always set alignment support scheme as dr_unaligned_supported for VMAT_ELEMENTWISE, it is true if we always adopt scalar stores, but as the above code shows, we could use vector stores for some cases, so we should use the correct alignment support scheme for it. This patch is to ensure the vector store is supported by further checking with vect_supportable_dr_alignment. The ICEs got exposed with patches moving costing next to the transform but they haven't been landed, the test coverage would be there once they get landed. The affected test cases are: - gcc.dg/vect/slp-45.c - gcc.dg/vect/vect-alias-check-{10,11,12}.c btw, I tried to make some correctness test case, but I realized that -mno-allow-movmisalign is mainly for noting movmisalign optab and it doesn't guard for the actual hw vector memory access insns, so I failed to make it unless I also altered some conditions for them as it. gcc/ChangeLog: * tree-vect-stmts.cc (vectorizable_store): Ensure the generated vector store for some case of VMAT_ELEMENTWISE is supported. --- gcc/tree-vect-stmts.cc | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)