* [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end @ 2021-12-17 10:05 Prathamesh Kulkarni 2021-12-17 11:07 ` Richard Sandiford 0 siblings, 1 reply; 15+ messages in thread From: Prathamesh Kulkarni @ 2021-12-17 10:05 UTC (permalink / raw) To: gcc Patches, Richard Sandiford, Richard Biener [-- Attachment #1: Type: text/plain, Size: 436 bytes --] Hi, The attached patch rearranges order of type-check for vec_perm_expr and relaxes type checking for lhs = vec_perm_expr<rhs1, rhs2, mask> when: rhs1 == rhs2, lhs is variable length vector, rhs1 is fixed length vector, TREE_TYPE (lhs) == TREE_TYPE (rhs1) I am not sure tho if this check is correct ? My intent was to capture case when vec_perm_expr is used to "extend" fixed length vector to it's VLA equivalent. Thanks, Prathamesh [-- Attachment #2: pr96463-3-midend.txt --] [-- Type: text/plain, Size: 3489 bytes --] diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 672e384ef09..9f91878c468 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -4325,10 +4325,11 @@ verify_gimple_assign_ternary (gassign *stmt) break; case VEC_PERM_EXPR: - if (!useless_type_conversion_p (lhs_type, rhs1_type) - || !useless_type_conversion_p (lhs_type, rhs2_type)) + if (TREE_CODE (rhs1_type) != VECTOR_TYPE + || TREE_CODE (rhs2_type) != VECTOR_TYPE + || TREE_CODE (rhs3_type) != VECTOR_TYPE) { - error ("type mismatch in %qs", code_name); + error ("vector types expected in %qs", code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); @@ -4336,11 +4337,14 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } - if (TREE_CODE (rhs1_type) != VECTOR_TYPE - || TREE_CODE (rhs2_type) != VECTOR_TYPE - || TREE_CODE (rhs3_type) != VECTOR_TYPE) + if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE + || (TREE_CODE (rhs3) != VECTOR_CST + && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE + (TREE_TYPE (rhs3_type))) + != GET_MODE_BITSIZE (SCALAR_TYPE_MODE + (TREE_TYPE (rhs1_type)))))) { - error ("vector types expected in %qs", code_name); + error ("invalid mask type in %qs", code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); @@ -4348,15 +4352,18 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } - if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), - TYPE_VECTOR_SUBPARTS (rhs2_type)) - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), - TYPE_VECTOR_SUBPARTS (rhs3_type)) - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), - TYPE_VECTOR_SUBPARTS (lhs_type))) + /* Accept lhs = vec_perm_expr<v, v, mask> if lhs is vector length agnostic, + and has same element type as v. */ + if (!TYPE_VECTOR_SUBPARTS (lhs_type).is_constant () + && operand_equal_p (rhs1, rhs2, 0) + && TYPE_VECTOR_SUBPARTS (rhs1_type).is_constant () + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type)) + return false; + + if (!useless_type_conversion_p (lhs_type, rhs1_type) + || !useless_type_conversion_p (lhs_type, rhs2_type)) { - error ("vectors with different element number found in %qs", - code_name); + error ("type mismatch in %qs", code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); @@ -4364,21 +4371,21 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } - if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE - || (TREE_CODE (rhs3) != VECTOR_CST - && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE - (TREE_TYPE (rhs3_type))) - != GET_MODE_BITSIZE (SCALAR_TYPE_MODE - (TREE_TYPE (rhs1_type)))))) + if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), + TYPE_VECTOR_SUBPARTS (rhs2_type)) + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), + TYPE_VECTOR_SUBPARTS (rhs3_type)) + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), + TYPE_VECTOR_SUBPARTS (lhs_type))) { - error ("invalid mask type in %qs", code_name); + error ("vectors with different element number found in %qs", + code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); debug_generic_expr (rhs3_type); return true; } - return false; case SAD_EXPR: ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2021-12-17 10:05 [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end Prathamesh Kulkarni @ 2021-12-17 11:07 ` Richard Sandiford 2021-12-27 10:25 ` Prathamesh Kulkarni 2022-01-03 10:40 ` Richard Biener 0 siblings, 2 replies; 15+ messages in thread From: Richard Sandiford @ 2021-12-17 11:07 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Richard Biener Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > Hi, > The attached patch rearranges order of type-check for vec_perm_expr > and relaxes type checking for > lhs = vec_perm_expr<rhs1, rhs2, mask> > > when: > rhs1 == rhs2, > lhs is variable length vector, > rhs1 is fixed length vector, > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > > I am not sure tho if this check is correct ? My intent was to capture > case when vec_perm_expr is used to "extend" fixed length vector to > it's VLA equivalent. VLAness isn't really the issue. We want the same thing to work for -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the vectors are fixed-length in that case. The principle is that for: A = VEC_PERM_EXPR <B, C, D>; the requirements are: - A, B, C and D must be vectors - A, B and C must have the same element type - D must have an integer element type - A and D must have the same number of elements (NA) - B and C must have the same number of elements (NB) The semantics are that we create a joined vector BC (all elements of B followed by all element of C) and that: A[i] = BC[D[i] % (NB+NB)] for 0 ≤ i < NA. This operation makes sense even if NA != NB. Thanks, Richard > > Thanks, > Prathamesh > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > index 672e384ef09..9f91878c468 100644 > --- a/gcc/tree-cfg.c > +++ b/gcc/tree-cfg.c > @@ -4325,10 +4325,11 @@ verify_gimple_assign_ternary (gassign *stmt) > break; > > case VEC_PERM_EXPR: > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > + if (TREE_CODE (rhs1_type) != VECTOR_TYPE > + || TREE_CODE (rhs2_type) != VECTOR_TYPE > + || TREE_CODE (rhs3_type) != VECTOR_TYPE) > { > - error ("type mismatch in %qs", code_name); > + error ("vector types expected in %qs", code_name); > debug_generic_expr (lhs_type); > debug_generic_expr (rhs1_type); > debug_generic_expr (rhs2_type); > @@ -4336,11 +4337,14 @@ verify_gimple_assign_ternary (gassign *stmt) > return true; > } > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > - || TREE_CODE (rhs2_type) != VECTOR_TYPE > - || TREE_CODE (rhs3_type) != VECTOR_TYPE) > + if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > + || (TREE_CODE (rhs3) != VECTOR_CST > + && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > + (TREE_TYPE (rhs3_type))) > + != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > + (TREE_TYPE (rhs1_type)))))) > { > - error ("vector types expected in %qs", code_name); > + error ("invalid mask type in %qs", code_name); > debug_generic_expr (lhs_type); > debug_generic_expr (rhs1_type); > debug_generic_expr (rhs2_type); > @@ -4348,15 +4352,18 @@ verify_gimple_assign_ternary (gassign *stmt) > return true; > } > > - if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > - TYPE_VECTOR_SUBPARTS (rhs2_type)) > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > - TYPE_VECTOR_SUBPARTS (rhs3_type)) > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > - TYPE_VECTOR_SUBPARTS (lhs_type))) > + /* Accept lhs = vec_perm_expr<v, v, mask> if lhs is vector length agnostic, > + and has same element type as v. */ > + if (!TYPE_VECTOR_SUBPARTS (lhs_type).is_constant () > + && operand_equal_p (rhs1, rhs2, 0) > + && TYPE_VECTOR_SUBPARTS (rhs1_type).is_constant () > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type)) > + return false; > + > + if (!useless_type_conversion_p (lhs_type, rhs1_type) > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > { > - error ("vectors with different element number found in %qs", > - code_name); > + error ("type mismatch in %qs", code_name); > debug_generic_expr (lhs_type); > debug_generic_expr (rhs1_type); > debug_generic_expr (rhs2_type); > @@ -4364,21 +4371,21 @@ verify_gimple_assign_ternary (gassign *stmt) > return true; > } > > - if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > - || (TREE_CODE (rhs3) != VECTOR_CST > - && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > - (TREE_TYPE (rhs3_type))) > - != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > - (TREE_TYPE (rhs1_type)))))) > + if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > + TYPE_VECTOR_SUBPARTS (rhs2_type)) > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > + TYPE_VECTOR_SUBPARTS (rhs3_type)) > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > + TYPE_VECTOR_SUBPARTS (lhs_type))) > { > - error ("invalid mask type in %qs", code_name); > + error ("vectors with different element number found in %qs", > + code_name); > debug_generic_expr (lhs_type); > debug_generic_expr (rhs1_type); > debug_generic_expr (rhs2_type); > debug_generic_expr (rhs3_type); > return true; > } > - > return false; > > case SAD_EXPR: ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2021-12-17 11:07 ` Richard Sandiford @ 2021-12-27 10:25 ` Prathamesh Kulkarni 2022-01-03 10:40 ` Richard Biener 1 sibling, 0 replies; 15+ messages in thread From: Prathamesh Kulkarni @ 2021-12-27 10:25 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Richard Biener, richard.sandiford [-- Attachment #1: Type: text/plain, Size: 5986 bytes --] On Fri, 17 Dec 2021 at 16:37, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > Hi, > > The attached patch rearranges order of type-check for vec_perm_expr > > and relaxes type checking for > > lhs = vec_perm_expr<rhs1, rhs2, mask> > > > > when: > > rhs1 == rhs2, > > lhs is variable length vector, > > rhs1 is fixed length vector, > > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > > > > I am not sure tho if this check is correct ? My intent was to capture > > case when vec_perm_expr is used to "extend" fixed length vector to > > it's VLA equivalent. > > VLAness isn't really the issue. We want the same thing to work for > -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > vectors are fixed-length in that case. > > The principle is that for: > > A = VEC_PERM_EXPR <B, C, D>; > > the requirements are: > > - A, B, C and D must be vectors > - A, B and C must have the same element type > - D must have an integer element type > - A and D must have the same number of elements (NA) > - B and C must have the same number of elements (NB) > > The semantics are that we create a joined vector BC (all elements of B > followed by all element of C) and that: > > A[i] = BC[D[i] % (NB+NB)] > > for 0 ≤ i < NA. > > This operation makes sense even if NA != NB. Thanks for the suggestions, I tried to modify the patch accordingly. Does it look OK ? Passes bootstrap+test on aarch64-linux-gnu. Thanks, Prathamesh > > Thanks, > Richard > > > > > Thanks, > > Prathamesh > > > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > > index 672e384ef09..9f91878c468 100644 > > --- a/gcc/tree-cfg.c > > +++ b/gcc/tree-cfg.c > > @@ -4325,10 +4325,11 @@ verify_gimple_assign_ternary (gassign *stmt) > > break; > > > > case VEC_PERM_EXPR: > > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > > + if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > + || TREE_CODE (rhs2_type) != VECTOR_TYPE > > + || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > { > > - error ("type mismatch in %qs", code_name); > > + error ("vector types expected in %qs", code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > @@ -4336,11 +4337,14 @@ verify_gimple_assign_ternary (gassign *stmt) > > return true; > > } > > > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > - || TREE_CODE (rhs2_type) != VECTOR_TYPE > > - || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > + if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > > + || (TREE_CODE (rhs3) != VECTOR_CST > > + && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > > + (TREE_TYPE (rhs3_type))) > > + != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > > + (TREE_TYPE (rhs1_type)))))) > > { > > - error ("vector types expected in %qs", code_name); > > + error ("invalid mask type in %qs", code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > @@ -4348,15 +4352,18 @@ verify_gimple_assign_ternary (gassign *stmt) > > return true; > > } > > > > - if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > - TYPE_VECTOR_SUBPARTS (rhs2_type)) > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > - TYPE_VECTOR_SUBPARTS (rhs3_type)) > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > > - TYPE_VECTOR_SUBPARTS (lhs_type))) > > + /* Accept lhs = vec_perm_expr<v, v, mask> if lhs is vector length agnostic, > > + and has same element type as v. */ > > + if (!TYPE_VECTOR_SUBPARTS (lhs_type).is_constant () > > + && operand_equal_p (rhs1, rhs2, 0) > > + && TYPE_VECTOR_SUBPARTS (rhs1_type).is_constant () > > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type)) > > + return false; > > + > > + if (!useless_type_conversion_p (lhs_type, rhs1_type) > > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > > { > > - error ("vectors with different element number found in %qs", > > - code_name); > > + error ("type mismatch in %qs", code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > @@ -4364,21 +4371,21 @@ verify_gimple_assign_ternary (gassign *stmt) > > return true; > > } > > > > - if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > > - || (TREE_CODE (rhs3) != VECTOR_CST > > - && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > > - (TREE_TYPE (rhs3_type))) > > - != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > > - (TREE_TYPE (rhs1_type)))))) > > + if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > + TYPE_VECTOR_SUBPARTS (rhs2_type)) > > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > + TYPE_VECTOR_SUBPARTS (rhs3_type)) > > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > > + TYPE_VECTOR_SUBPARTS (lhs_type))) > > { > > - error ("invalid mask type in %qs", code_name); > > + error ("vectors with different element number found in %qs", > > + code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > debug_generic_expr (rhs3_type); > > return true; > > } > > - > > return false; > > > > case SAD_EXPR: [-- Attachment #2: pr96463-4-midend.txt --] [-- Type: text/plain, Size: 3654 bytes --] diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 672e384ef09..acc835c77cc 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -4325,10 +4325,12 @@ verify_gimple_assign_ternary (gassign *stmt) break; case VEC_PERM_EXPR: - if (!useless_type_conversion_p (lhs_type, rhs1_type) - || !useless_type_conversion_p (lhs_type, rhs2_type)) + if (TREE_CODE (lhs_type) != VECTOR_TYPE + || TREE_CODE (rhs1_type) != VECTOR_TYPE + || TREE_CODE (rhs2_type) != VECTOR_TYPE + || TREE_CODE (rhs3_type) != VECTOR_TYPE) { - error ("type mismatch in %qs", code_name); + error ("vector types expected in %qs", code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); @@ -4336,11 +4338,14 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } - if (TREE_CODE (rhs1_type) != VECTOR_TYPE - || TREE_CODE (rhs2_type) != VECTOR_TYPE - || TREE_CODE (rhs3_type) != VECTOR_TYPE) + if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE + || (TREE_CODE (rhs3) != VECTOR_CST + && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE + (TREE_TYPE (rhs3_type))) + != GET_MODE_BITSIZE (SCALAR_TYPE_MODE + (TREE_TYPE (rhs1_type)))))) { - error ("vector types expected in %qs", code_name); + error ("invalid mask type in %qs", code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); @@ -4348,15 +4353,23 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } - if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), - TYPE_VECTOR_SUBPARTS (rhs2_type)) - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), - TYPE_VECTOR_SUBPARTS (rhs3_type)) - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), - TYPE_VECTOR_SUBPARTS (lhs_type))) + /* If lhs and rhs have different types, check that: + (a) Elements have same type. + (b) lhs length == rhs3 length. + (c) rhs1 length == rhs2 length. */ + + if (!(types_compatible_p (lhs_type, rhs1_type) + && types_compatible_p (lhs_type, rhs2_type)) + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) + return false; + + if (!useless_type_conversion_p (lhs_type, rhs1_type) + || !useless_type_conversion_p (lhs_type, rhs2_type)) { - error ("vectors with different element number found in %qs", - code_name); + error ("type mismatch in %qs", code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); @@ -4364,14 +4377,15 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } - if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE - || (TREE_CODE (rhs3) != VECTOR_CST - && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE - (TREE_TYPE (rhs3_type))) - != GET_MODE_BITSIZE (SCALAR_TYPE_MODE - (TREE_TYPE (rhs1_type)))))) + if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), + TYPE_VECTOR_SUBPARTS (rhs2_type)) + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), + TYPE_VECTOR_SUBPARTS (rhs3_type)) + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), + TYPE_VECTOR_SUBPARTS (lhs_type))) { - error ("invalid mask type in %qs", code_name); + error ("vectors with different element number found in %qs", + code_name); debug_generic_expr (lhs_type); debug_generic_expr (rhs1_type); debug_generic_expr (rhs2_type); ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2021-12-17 11:07 ` Richard Sandiford 2021-12-27 10:25 ` Prathamesh Kulkarni @ 2022-01-03 10:40 ` Richard Biener 2022-01-04 11:50 ` Richard Sandiford 1 sibling, 1 reply; 15+ messages in thread From: Richard Biener @ 2022-01-03 10:40 UTC (permalink / raw) To: Richard Sandiford; +Cc: Prathamesh Kulkarni, gcc Patches On Fri, 17 Dec 2021, Richard Sandiford wrote: > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > Hi, > > The attached patch rearranges order of type-check for vec_perm_expr > > and relaxes type checking for > > lhs = vec_perm_expr<rhs1, rhs2, mask> > > > > when: > > rhs1 == rhs2, > > lhs is variable length vector, > > rhs1 is fixed length vector, > > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > > > > I am not sure tho if this check is correct ? My intent was to capture > > case when vec_perm_expr is used to "extend" fixed length vector to > > it's VLA equivalent. > > VLAness isn't really the issue. We want the same thing to work for > -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > vectors are fixed-length in that case. > > The principle is that for: > > A = VEC_PERM_EXPR <B, C, D>; > > the requirements are: > > - A, B, C and D must be vectors > - A, B and C must have the same element type > - D must have an integer element type > - A and D must have the same number of elements (NA) > - B and C must have the same number of elements (NB) > > The semantics are that we create a joined vector BC (all elements of B > followed by all element of C) and that: > > A[i] = BC[D[i] % (NB+NB)] > > for 0 ≤ i < NA. > > This operation makes sense even if NA != NB. But note that we don't currently expect NA != NB and the optab just has a single mode. I'd rather go with the simpler patch I posted as reply to the earlier mail rather such large refactoring at this point. Richard. > Thanks, > Richard > > > > > Thanks, > > Prathamesh > > > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > > index 672e384ef09..9f91878c468 100644 > > --- a/gcc/tree-cfg.c > > +++ b/gcc/tree-cfg.c > > @@ -4325,10 +4325,11 @@ verify_gimple_assign_ternary (gassign *stmt) > > break; > > > > case VEC_PERM_EXPR: > > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > > + if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > + || TREE_CODE (rhs2_type) != VECTOR_TYPE > > + || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > { > > - error ("type mismatch in %qs", code_name); > > + error ("vector types expected in %qs", code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > @@ -4336,11 +4337,14 @@ verify_gimple_assign_ternary (gassign *stmt) > > return true; > > } > > > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > - || TREE_CODE (rhs2_type) != VECTOR_TYPE > > - || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > + if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > > + || (TREE_CODE (rhs3) != VECTOR_CST > > + && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > > + (TREE_TYPE (rhs3_type))) > > + != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > > + (TREE_TYPE (rhs1_type)))))) > > { > > - error ("vector types expected in %qs", code_name); > > + error ("invalid mask type in %qs", code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > @@ -4348,15 +4352,18 @@ verify_gimple_assign_ternary (gassign *stmt) > > return true; > > } > > > > - if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > - TYPE_VECTOR_SUBPARTS (rhs2_type)) > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > - TYPE_VECTOR_SUBPARTS (rhs3_type)) > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > > - TYPE_VECTOR_SUBPARTS (lhs_type))) > > + /* Accept lhs = vec_perm_expr<v, v, mask> if lhs is vector length agnostic, > > + and has same element type as v. */ > > + if (!TYPE_VECTOR_SUBPARTS (lhs_type).is_constant () > > + && operand_equal_p (rhs1, rhs2, 0) > > + && TYPE_VECTOR_SUBPARTS (rhs1_type).is_constant () > > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type)) > > + return false; > > + > > + if (!useless_type_conversion_p (lhs_type, rhs1_type) > > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > > { > > - error ("vectors with different element number found in %qs", > > - code_name); > > + error ("type mismatch in %qs", code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > @@ -4364,21 +4371,21 @@ verify_gimple_assign_ternary (gassign *stmt) > > return true; > > } > > > > - if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > > - || (TREE_CODE (rhs3) != VECTOR_CST > > - && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > > - (TREE_TYPE (rhs3_type))) > > - != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > > - (TREE_TYPE (rhs1_type)))))) > > + if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > + TYPE_VECTOR_SUBPARTS (rhs2_type)) > > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > + TYPE_VECTOR_SUBPARTS (rhs3_type)) > > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > > + TYPE_VECTOR_SUBPARTS (lhs_type))) > > { > > - error ("invalid mask type in %qs", code_name); > > + error ("vectors with different element number found in %qs", > > + code_name); > > debug_generic_expr (lhs_type); > > debug_generic_expr (rhs1_type); > > debug_generic_expr (rhs2_type); > > debug_generic_expr (rhs3_type); > > return true; > > } > > - > > return false; > > > > case SAD_EXPR: > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-01-03 10:40 ` Richard Biener @ 2022-01-04 11:50 ` Richard Sandiford 2022-01-04 12:40 ` Richard Biener 0 siblings, 1 reply; 15+ messages in thread From: Richard Sandiford @ 2022-01-04 11:50 UTC (permalink / raw) To: Richard Biener; +Cc: Prathamesh Kulkarni, gcc Patches Richard Biener <rguenther@suse.de> writes: > On Fri, 17 Dec 2021, Richard Sandiford wrote: > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> > Hi, >> > The attached patch rearranges order of type-check for vec_perm_expr >> > and relaxes type checking for >> > lhs = vec_perm_expr<rhs1, rhs2, mask> >> > >> > when: >> > rhs1 == rhs2, >> > lhs is variable length vector, >> > rhs1 is fixed length vector, >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) >> > >> > I am not sure tho if this check is correct ? My intent was to capture >> > case when vec_perm_expr is used to "extend" fixed length vector to >> > it's VLA equivalent. >> >> VLAness isn't really the issue. We want the same thing to work for >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the >> vectors are fixed-length in that case. >> >> The principle is that for: >> >> A = VEC_PERM_EXPR <B, C, D>; >> >> the requirements are: >> >> - A, B, C and D must be vectors >> - A, B and C must have the same element type >> - D must have an integer element type >> - A and D must have the same number of elements (NA) >> - B and C must have the same number of elements (NB) >> >> The semantics are that we create a joined vector BC (all elements of B >> followed by all element of C) and that: >> >> A[i] = BC[D[i] % (NB+NB)] >> >> for 0 ≤ i < NA. >> >> This operation makes sense even if NA != NB. > > But note that we don't currently expect NA != NB and the optab just > has a single mode. True, but we only need this for constant permutes. They are already special in that they allow the index elements to be wider than the data elements. Thanks, Richard > > I'd rather go with the simpler patch I posted as reply to the earlier > mail rather such large refactoring at this point. > > Richard. > >> Thanks, >> Richard >> >> > >> > Thanks, >> > Prathamesh >> > >> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c >> > index 672e384ef09..9f91878c468 100644 >> > --- a/gcc/tree-cfg.c >> > +++ b/gcc/tree-cfg.c >> > @@ -4325,10 +4325,11 @@ verify_gimple_assign_ternary (gassign *stmt) >> > break; >> > >> > case VEC_PERM_EXPR: >> > - if (!useless_type_conversion_p (lhs_type, rhs1_type) >> > - || !useless_type_conversion_p (lhs_type, rhs2_type)) >> > + if (TREE_CODE (rhs1_type) != VECTOR_TYPE >> > + || TREE_CODE (rhs2_type) != VECTOR_TYPE >> > + || TREE_CODE (rhs3_type) != VECTOR_TYPE) >> > { >> > - error ("type mismatch in %qs", code_name); >> > + error ("vector types expected in %qs", code_name); >> > debug_generic_expr (lhs_type); >> > debug_generic_expr (rhs1_type); >> > debug_generic_expr (rhs2_type); >> > @@ -4336,11 +4337,14 @@ verify_gimple_assign_ternary (gassign *stmt) >> > return true; >> > } >> > >> > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE >> > - || TREE_CODE (rhs2_type) != VECTOR_TYPE >> > - || TREE_CODE (rhs3_type) != VECTOR_TYPE) >> > + if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE >> > + || (TREE_CODE (rhs3) != VECTOR_CST >> > + && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE >> > + (TREE_TYPE (rhs3_type))) >> > + != GET_MODE_BITSIZE (SCALAR_TYPE_MODE >> > + (TREE_TYPE (rhs1_type)))))) >> > { >> > - error ("vector types expected in %qs", code_name); >> > + error ("invalid mask type in %qs", code_name); >> > debug_generic_expr (lhs_type); >> > debug_generic_expr (rhs1_type); >> > debug_generic_expr (rhs2_type); >> > @@ -4348,15 +4352,18 @@ verify_gimple_assign_ternary (gassign *stmt) >> > return true; >> > } >> > >> > - if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), >> > - TYPE_VECTOR_SUBPARTS (rhs2_type)) >> > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), >> > - TYPE_VECTOR_SUBPARTS (rhs3_type)) >> > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), >> > - TYPE_VECTOR_SUBPARTS (lhs_type))) >> > + /* Accept lhs = vec_perm_expr<v, v, mask> if lhs is vector length agnostic, >> > + and has same element type as v. */ >> > + if (!TYPE_VECTOR_SUBPARTS (lhs_type).is_constant () >> > + && operand_equal_p (rhs1, rhs2, 0) >> > + && TYPE_VECTOR_SUBPARTS (rhs1_type).is_constant () >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type)) >> > + return false; >> > + >> > + if (!useless_type_conversion_p (lhs_type, rhs1_type) >> > + || !useless_type_conversion_p (lhs_type, rhs2_type)) >> > { >> > - error ("vectors with different element number found in %qs", >> > - code_name); >> > + error ("type mismatch in %qs", code_name); >> > debug_generic_expr (lhs_type); >> > debug_generic_expr (rhs1_type); >> > debug_generic_expr (rhs2_type); >> > @@ -4364,21 +4371,21 @@ verify_gimple_assign_ternary (gassign *stmt) >> > return true; >> > } >> > >> > - if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE >> > - || (TREE_CODE (rhs3) != VECTOR_CST >> > - && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE >> > - (TREE_TYPE (rhs3_type))) >> > - != GET_MODE_BITSIZE (SCALAR_TYPE_MODE >> > - (TREE_TYPE (rhs1_type)))))) >> > + if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), >> > + TYPE_VECTOR_SUBPARTS (rhs2_type)) >> > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), >> > + TYPE_VECTOR_SUBPARTS (rhs3_type)) >> > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), >> > + TYPE_VECTOR_SUBPARTS (lhs_type))) >> > { >> > - error ("invalid mask type in %qs", code_name); >> > + error ("vectors with different element number found in %qs", >> > + code_name); >> > debug_generic_expr (lhs_type); >> > debug_generic_expr (rhs1_type); >> > debug_generic_expr (rhs2_type); >> > debug_generic_expr (rhs3_type); >> > return true; >> > } >> > - >> > return false; >> > >> > case SAD_EXPR: >> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-01-04 11:50 ` Richard Sandiford @ 2022-01-04 12:40 ` Richard Biener 2022-01-04 13:42 ` Richard Sandiford 0 siblings, 1 reply; 15+ messages in thread From: Richard Biener @ 2022-01-04 12:40 UTC (permalink / raw) To: Richard Sandiford; +Cc: Prathamesh Kulkarni, gcc Patches On Tue, 4 Jan 2022, Richard Sandiford wrote: > Richard Biener <rguenther@suse.de> writes: > > On Fri, 17 Dec 2021, Richard Sandiford wrote: > > > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> > Hi, > >> > The attached patch rearranges order of type-check for vec_perm_expr > >> > and relaxes type checking for > >> > lhs = vec_perm_expr<rhs1, rhs2, mask> > >> > > >> > when: > >> > rhs1 == rhs2, > >> > lhs is variable length vector, > >> > rhs1 is fixed length vector, > >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > >> > > >> > I am not sure tho if this check is correct ? My intent was to capture > >> > case when vec_perm_expr is used to "extend" fixed length vector to > >> > it's VLA equivalent. > >> > >> VLAness isn't really the issue. We want the same thing to work for > >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > >> vectors are fixed-length in that case. > >> > >> The principle is that for: > >> > >> A = VEC_PERM_EXPR <B, C, D>; > >> > >> the requirements are: > >> > >> - A, B, C and D must be vectors > >> - A, B and C must have the same element type > >> - D must have an integer element type > >> - A and D must have the same number of elements (NA) > >> - B and C must have the same number of elements (NB) > >> > >> The semantics are that we create a joined vector BC (all elements of B > >> followed by all element of C) and that: > >> > >> A[i] = BC[D[i] % (NB+NB)] > >> > >> for 0 ≤ i < NA. > >> > >> This operation makes sense even if NA != NB. > > > > But note that we don't currently expect NA != NB and the optab just > > has a single mode. > > True, but we only need this for constant permutes. They are already > special in that they allow the index elements to be wider than the data > elements. OK, then we should reflect this in the stmt verification and only relax the constant permute vector case and also amend the TARGET_VECTORIZE_VEC_PERM_CONST accordingly. For non-constant permutes the docs say the mode of vec_perm is the common mode of operands 1 and 2 whilst the mode of operand 0 is unspecified - even unconstrained by the docs. I'm not sure if vec_perm expansion is expected to eventually FAIL. Updating the docs of vec_perm would be appreciated as well. As said I prefer to not mangle the existing stmt checking too much at this stage so minimal adjustment is prefered there. Thanks, Richard. > Thanks, > Richard > > > > > I'd rather go with the simpler patch I posted as reply to the earlier > > mail rather such large refactoring at this point. > > > > Richard. > > > >> Thanks, > >> Richard > >> > >> > > >> > Thanks, > >> > Prathamesh > >> > > >> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c > >> > index 672e384ef09..9f91878c468 100644 > >> > --- a/gcc/tree-cfg.c > >> > +++ b/gcc/tree-cfg.c > >> > @@ -4325,10 +4325,11 @@ verify_gimple_assign_ternary (gassign *stmt) > >> > break; > >> > > >> > case VEC_PERM_EXPR: > >> > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > >> > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > >> > + if (TREE_CODE (rhs1_type) != VECTOR_TYPE > >> > + || TREE_CODE (rhs2_type) != VECTOR_TYPE > >> > + || TREE_CODE (rhs3_type) != VECTOR_TYPE) > >> > { > >> > - error ("type mismatch in %qs", code_name); > >> > + error ("vector types expected in %qs", code_name); > >> > debug_generic_expr (lhs_type); > >> > debug_generic_expr (rhs1_type); > >> > debug_generic_expr (rhs2_type); > >> > @@ -4336,11 +4337,14 @@ verify_gimple_assign_ternary (gassign *stmt) > >> > return true; > >> > } > >> > > >> > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > >> > - || TREE_CODE (rhs2_type) != VECTOR_TYPE > >> > - || TREE_CODE (rhs3_type) != VECTOR_TYPE) > >> > + if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > >> > + || (TREE_CODE (rhs3) != VECTOR_CST > >> > + && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > >> > + (TREE_TYPE (rhs3_type))) > >> > + != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > >> > + (TREE_TYPE (rhs1_type)))))) > >> > { > >> > - error ("vector types expected in %qs", code_name); > >> > + error ("invalid mask type in %qs", code_name); > >> > debug_generic_expr (lhs_type); > >> > debug_generic_expr (rhs1_type); > >> > debug_generic_expr (rhs2_type); > >> > @@ -4348,15 +4352,18 @@ verify_gimple_assign_ternary (gassign *stmt) > >> > return true; > >> > } > >> > > >> > - if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > >> > - TYPE_VECTOR_SUBPARTS (rhs2_type)) > >> > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > >> > - TYPE_VECTOR_SUBPARTS (rhs3_type)) > >> > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > >> > - TYPE_VECTOR_SUBPARTS (lhs_type))) > >> > + /* Accept lhs = vec_perm_expr<v, v, mask> if lhs is vector length agnostic, > >> > + and has same element type as v. */ > >> > + if (!TYPE_VECTOR_SUBPARTS (lhs_type).is_constant () > >> > + && operand_equal_p (rhs1, rhs2, 0) > >> > + && TYPE_VECTOR_SUBPARTS (rhs1_type).is_constant () > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type)) > >> > + return false; > >> > + > >> > + if (!useless_type_conversion_p (lhs_type, rhs1_type) > >> > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > >> > { > >> > - error ("vectors with different element number found in %qs", > >> > - code_name); > >> > + error ("type mismatch in %qs", code_name); > >> > debug_generic_expr (lhs_type); > >> > debug_generic_expr (rhs1_type); > >> > debug_generic_expr (rhs2_type); > >> > @@ -4364,21 +4371,21 @@ verify_gimple_assign_ternary (gassign *stmt) > >> > return true; > >> > } > >> > > >> > - if (TREE_CODE (TREE_TYPE (rhs3_type)) != INTEGER_TYPE > >> > - || (TREE_CODE (rhs3) != VECTOR_CST > >> > - && (GET_MODE_BITSIZE (SCALAR_INT_TYPE_MODE > >> > - (TREE_TYPE (rhs3_type))) > >> > - != GET_MODE_BITSIZE (SCALAR_TYPE_MODE > >> > - (TREE_TYPE (rhs1_type)))))) > >> > + if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > >> > + TYPE_VECTOR_SUBPARTS (rhs2_type)) > >> > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > >> > + TYPE_VECTOR_SUBPARTS (rhs3_type)) > >> > + || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > >> > + TYPE_VECTOR_SUBPARTS (lhs_type))) > >> > { > >> > - error ("invalid mask type in %qs", code_name); > >> > + error ("vectors with different element number found in %qs", > >> > + code_name); > >> > debug_generic_expr (lhs_type); > >> > debug_generic_expr (rhs1_type); > >> > debug_generic_expr (rhs2_type); > >> > debug_generic_expr (rhs3_type); > >> > return true; > >> > } > >> > - > >> > return false; > >> > > >> > case SAD_EXPR: > >> > -- Richard Biener <rguenther@suse.de> SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg) ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-01-04 12:40 ` Richard Biener @ 2022-01-04 13:42 ` Richard Sandiford 2022-05-03 10:41 ` Prathamesh Kulkarni 0 siblings, 1 reply; 15+ messages in thread From: Richard Sandiford @ 2022-01-04 13:42 UTC (permalink / raw) To: Richard Biener; +Cc: Prathamesh Kulkarni, gcc Patches Richard Biener <rguenther@suse.de> writes: > On Tue, 4 Jan 2022, Richard Sandiford wrote: > >> Richard Biener <rguenther@suse.de> writes: >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: >> > >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> >> > Hi, >> >> > The attached patch rearranges order of type-check for vec_perm_expr >> >> > and relaxes type checking for >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> >> >> > >> >> > when: >> >> > rhs1 == rhs2, >> >> > lhs is variable length vector, >> >> > rhs1 is fixed length vector, >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) >> >> > >> >> > I am not sure tho if this check is correct ? My intent was to capture >> >> > case when vec_perm_expr is used to "extend" fixed length vector to >> >> > it's VLA equivalent. >> >> >> >> VLAness isn't really the issue. We want the same thing to work for >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the >> >> vectors are fixed-length in that case. >> >> >> >> The principle is that for: >> >> >> >> A = VEC_PERM_EXPR <B, C, D>; >> >> >> >> the requirements are: >> >> >> >> - A, B, C and D must be vectors >> >> - A, B and C must have the same element type >> >> - D must have an integer element type >> >> - A and D must have the same number of elements (NA) >> >> - B and C must have the same number of elements (NB) >> >> >> >> The semantics are that we create a joined vector BC (all elements of B >> >> followed by all element of C) and that: >> >> >> >> A[i] = BC[D[i] % (NB+NB)] >> >> >> >> for 0 ≤ i < NA. >> >> >> >> This operation makes sense even if NA != NB. >> > >> > But note that we don't currently expect NA != NB and the optab just >> > has a single mode. >> >> True, but we only need this for constant permutes. They are already >> special in that they allow the index elements to be wider than the data >> elements. > > OK, then we should reflect this in the stmt verification and only relax > the constant permute vector case and also amend the > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. Sounds good. > For non-constant permutes the docs say the mode of vec_perm is > the common mode of operands 1 and 2 whilst the mode of operand 0 > is unspecified - even unconstrained by the docs. I'm not sure > if vec_perm expansion is expected to eventually FAIL. Updating the > docs of vec_perm would be appreciated as well. Yeah, I guess de facto operand 0 has to be the same mode as operands 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious or self-explanatory at the time. :-) > As said I prefer to not mangle the existing stmt checking too much > at this stage so minimal adjustment is prefered there. The PR is only an enhancement request rather than a bug, so I think the patch would need to wait for GCC 13 whatever happens. Thanks, Richard ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-01-04 13:42 ` Richard Sandiford @ 2022-05-03 10:41 ` Prathamesh Kulkarni 2022-05-03 12:55 ` Richard Sandiford 0 siblings, 1 reply; 15+ messages in thread From: Prathamesh Kulkarni @ 2022-05-03 10:41 UTC (permalink / raw) To: Richard Biener, Prathamesh Kulkarni, gcc Patches, richard.sandiford [-- Attachment #1: Type: text/plain, Size: 3276 bytes --] On Tue, 4 Jan 2022 at 19:12, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Richard Biener <rguenther@suse.de> writes: > > On Tue, 4 Jan 2022, Richard Sandiford wrote: > > > >> Richard Biener <rguenther@suse.de> writes: > >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: > >> > > >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> >> > Hi, > >> >> > The attached patch rearranges order of type-check for vec_perm_expr > >> >> > and relaxes type checking for > >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> > >> >> > > >> >> > when: > >> >> > rhs1 == rhs2, > >> >> > lhs is variable length vector, > >> >> > rhs1 is fixed length vector, > >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > >> >> > > >> >> > I am not sure tho if this check is correct ? My intent was to capture > >> >> > case when vec_perm_expr is used to "extend" fixed length vector to > >> >> > it's VLA equivalent. > >> >> > >> >> VLAness isn't really the issue. We want the same thing to work for > >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > >> >> vectors are fixed-length in that case. > >> >> > >> >> The principle is that for: > >> >> > >> >> A = VEC_PERM_EXPR <B, C, D>; > >> >> > >> >> the requirements are: > >> >> > >> >> - A, B, C and D must be vectors > >> >> - A, B and C must have the same element type > >> >> - D must have an integer element type > >> >> - A and D must have the same number of elements (NA) > >> >> - B and C must have the same number of elements (NB) > >> >> > >> >> The semantics are that we create a joined vector BC (all elements of B > >> >> followed by all element of C) and that: > >> >> > >> >> A[i] = BC[D[i] % (NB+NB)] > >> >> > >> >> for 0 ≤ i < NA. > >> >> > >> >> This operation makes sense even if NA != NB. > >> > > >> > But note that we don't currently expect NA != NB and the optab just > >> > has a single mode. > >> > >> True, but we only need this for constant permutes. They are already > >> special in that they allow the index elements to be wider than the data > >> elements. > > > > OK, then we should reflect this in the stmt verification and only relax > > the constant permute vector case and also amend the > > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. > > Sounds good. > > > For non-constant permutes the docs say the mode of vec_perm is > > the common mode of operands 1 and 2 whilst the mode of operand 0 > > is unspecified - even unconstrained by the docs. I'm not sure > > if vec_perm expansion is expected to eventually FAIL. Updating the > > docs of vec_perm would be appreciated as well. > > Yeah, I guess de facto operand 0 has to be the same mode as operands > 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious > or self-explanatory at the time. :-) > > > As said I prefer to not mangle the existing stmt checking too much > > at this stage so minimal adjustment is prefered there. > > The PR is only an enhancement request rather than a bug, so I think the > patch would need to wait for GCC 13 whatever happens. Hi, In attached patch, the type checking is relaxed only if mask is constant. Does this look OK ? Thanks, Prathamesh > > Thanks, > Richard [-- Attachment #2: pr96463-5-midend.txt --] [-- Type: text/plain, Size: 1136 bytes --] diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index e321d929fd0..02b88f67855 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) break; case VEC_PERM_EXPR: + /* If permute is constant, then we allow for lhs and rhs + to have different vector types, provided: + (1) lhs, rhs1, rhs2, and rhs3 have same element type. + (2) rhs3 vector has integer element type. + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ + + if (TREE_CONSTANT (rhs3) + && VECTOR_TYPE_P (lhs_type) + && VECTOR_TYPE_P (rhs1_type) + && VECTOR_TYPE_P (rhs2_type) + && VECTOR_TYPE_P (rhs3_type) + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) + return false; + if (!useless_type_conversion_p (lhs_type, rhs1_type) || !useless_type_conversion_p (lhs_type, rhs2_type)) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-05-03 10:41 ` Prathamesh Kulkarni @ 2022-05-03 12:55 ` Richard Sandiford 2022-05-09 11:21 ` Prathamesh Kulkarni 0 siblings, 1 reply; 15+ messages in thread From: Richard Sandiford @ 2022-05-03 12:55 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Richard Biener, gcc Patches Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > On Tue, 4 Jan 2022 at 19:12, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Richard Biener <rguenther@suse.de> writes: >> > On Tue, 4 Jan 2022, Richard Sandiford wrote: >> > >> >> Richard Biener <rguenther@suse.de> writes: >> >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: >> >> > >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> >> >> > Hi, >> >> >> > The attached patch rearranges order of type-check for vec_perm_expr >> >> >> > and relaxes type checking for >> >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> >> >> >> > >> >> >> > when: >> >> >> > rhs1 == rhs2, >> >> >> > lhs is variable length vector, >> >> >> > rhs1 is fixed length vector, >> >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) >> >> >> > >> >> >> > I am not sure tho if this check is correct ? My intent was to capture >> >> >> > case when vec_perm_expr is used to "extend" fixed length vector to >> >> >> > it's VLA equivalent. >> >> >> >> >> >> VLAness isn't really the issue. We want the same thing to work for >> >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the >> >> >> vectors are fixed-length in that case. >> >> >> >> >> >> The principle is that for: >> >> >> >> >> >> A = VEC_PERM_EXPR <B, C, D>; >> >> >> >> >> >> the requirements are: >> >> >> >> >> >> - A, B, C and D must be vectors >> >> >> - A, B and C must have the same element type >> >> >> - D must have an integer element type >> >> >> - A and D must have the same number of elements (NA) >> >> >> - B and C must have the same number of elements (NB) >> >> >> >> >> >> The semantics are that we create a joined vector BC (all elements of B >> >> >> followed by all element of C) and that: >> >> >> >> >> >> A[i] = BC[D[i] % (NB+NB)] >> >> >> >> >> >> for 0 ≤ i < NA. >> >> >> >> >> >> This operation makes sense even if NA != NB. >> >> > >> >> > But note that we don't currently expect NA != NB and the optab just >> >> > has a single mode. >> >> >> >> True, but we only need this for constant permutes. They are already >> >> special in that they allow the index elements to be wider than the data >> >> elements. >> > >> > OK, then we should reflect this in the stmt verification and only relax >> > the constant permute vector case and also amend the >> > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. >> >> Sounds good. >> >> > For non-constant permutes the docs say the mode of vec_perm is >> > the common mode of operands 1 and 2 whilst the mode of operand 0 >> > is unspecified - even unconstrained by the docs. I'm not sure >> > if vec_perm expansion is expected to eventually FAIL. Updating the >> > docs of vec_perm would be appreciated as well. >> >> Yeah, I guess de facto operand 0 has to be the same mode as operands >> 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious >> or self-explanatory at the time. :-) >> >> > As said I prefer to not mangle the existing stmt checking too much >> > at this stage so minimal adjustment is prefered there. >> >> The PR is only an enhancement request rather than a bug, so I think the >> patch would need to wait for GCC 13 whatever happens. > Hi, > In attached patch, the type checking is relaxed only if mask is constant. > Does this look OK ? > > Thanks, > Prathamesh >> >> Thanks, >> Richard > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > index e321d929fd0..02b88f67855 100644 > --- a/gcc/tree-cfg.cc > +++ b/gcc/tree-cfg.cc > @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) > break; > > case VEC_PERM_EXPR: > + /* If permute is constant, then we allow for lhs and rhs > + to have different vector types, provided: > + (1) lhs, rhs1, rhs2, and rhs3 have same element type. This isn't a requirement for rhs3. > + (2) rhs3 vector has integer element type. > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > + > + if (TREE_CONSTANT (rhs3) > + && VECTOR_TYPE_P (lhs_type) > + && VECTOR_TYPE_P (rhs1_type) > + && VECTOR_TYPE_P (rhs2_type) > + && VECTOR_TYPE_P (rhs3_type) > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) > + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) > + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) > + return false; > + I think this should be integrated into the existing conditions rather than done as an initial special case. It might make sense to start with: if (TREE_CODE (rhs1_type) != VECTOR_TYPE || TREE_CODE (rhs2_type) != VECTOR_TYPE || TREE_CODE (rhs3_type) != VECTOR_TYPE) { but expanded to test lhs_type too. Then the other parts of the new test should be distributed across the existing conditions. The type tests should use useless_type_conversion_p rather than ==. Thanks, Richard > if (!useless_type_conversion_p (lhs_type, rhs1_type) > || !useless_type_conversion_p (lhs_type, rhs2_type)) > { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-05-03 12:55 ` Richard Sandiford @ 2022-05-09 11:21 ` Prathamesh Kulkarni 2022-05-09 13:52 ` Richard Sandiford 0 siblings, 1 reply; 15+ messages in thread From: Prathamesh Kulkarni @ 2022-05-09 11:21 UTC (permalink / raw) To: Prathamesh Kulkarni, Richard Biener, gcc Patches, richard.sandiford [-- Attachment #1: Type: text/plain, Size: 6034 bytes --] On Tue, 3 May 2022 at 18:25, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > On Tue, 4 Jan 2022 at 19:12, Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> Richard Biener <rguenther@suse.de> writes: > >> > On Tue, 4 Jan 2022, Richard Sandiford wrote: > >> > > >> >> Richard Biener <rguenther@suse.de> writes: > >> >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: > >> >> > > >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> >> >> > Hi, > >> >> >> > The attached patch rearranges order of type-check for vec_perm_expr > >> >> >> > and relaxes type checking for > >> >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> > >> >> >> > > >> >> >> > when: > >> >> >> > rhs1 == rhs2, > >> >> >> > lhs is variable length vector, > >> >> >> > rhs1 is fixed length vector, > >> >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > >> >> >> > > >> >> >> > I am not sure tho if this check is correct ? My intent was to capture > >> >> >> > case when vec_perm_expr is used to "extend" fixed length vector to > >> >> >> > it's VLA equivalent. > >> >> >> > >> >> >> VLAness isn't really the issue. We want the same thing to work for > >> >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > >> >> >> vectors are fixed-length in that case. > >> >> >> > >> >> >> The principle is that for: > >> >> >> > >> >> >> A = VEC_PERM_EXPR <B, C, D>; > >> >> >> > >> >> >> the requirements are: > >> >> >> > >> >> >> - A, B, C and D must be vectors > >> >> >> - A, B and C must have the same element type > >> >> >> - D must have an integer element type > >> >> >> - A and D must have the same number of elements (NA) > >> >> >> - B and C must have the same number of elements (NB) > >> >> >> > >> >> >> The semantics are that we create a joined vector BC (all elements of B > >> >> >> followed by all element of C) and that: > >> >> >> > >> >> >> A[i] = BC[D[i] % (NB+NB)] > >> >> >> > >> >> >> for 0 ≤ i < NA. > >> >> >> > >> >> >> This operation makes sense even if NA != NB. > >> >> > > >> >> > But note that we don't currently expect NA != NB and the optab just > >> >> > has a single mode. > >> >> > >> >> True, but we only need this for constant permutes. They are already > >> >> special in that they allow the index elements to be wider than the data > >> >> elements. > >> > > >> > OK, then we should reflect this in the stmt verification and only relax > >> > the constant permute vector case and also amend the > >> > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. > >> > >> Sounds good. > >> > >> > For non-constant permutes the docs say the mode of vec_perm is > >> > the common mode of operands 1 and 2 whilst the mode of operand 0 > >> > is unspecified - even unconstrained by the docs. I'm not sure > >> > if vec_perm expansion is expected to eventually FAIL. Updating the > >> > docs of vec_perm would be appreciated as well. > >> > >> Yeah, I guess de facto operand 0 has to be the same mode as operands > >> 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious > >> or self-explanatory at the time. :-) > >> > >> > As said I prefer to not mangle the existing stmt checking too much > >> > at this stage so minimal adjustment is prefered there. > >> > >> The PR is only an enhancement request rather than a bug, so I think the > >> patch would need to wait for GCC 13 whatever happens. > > Hi, > > In attached patch, the type checking is relaxed only if mask is constant. > > Does this look OK ? > > > > Thanks, > > Prathamesh > >> > >> Thanks, > >> Richard > > > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > index e321d929fd0..02b88f67855 100644 > > --- a/gcc/tree-cfg.cc > > +++ b/gcc/tree-cfg.cc > > @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) > > break; > > > > case VEC_PERM_EXPR: > > + /* If permute is constant, then we allow for lhs and rhs > > + to have different vector types, provided: > > + (1) lhs, rhs1, rhs2, and rhs3 have same element type. > > This isn't a requirement for rhs3. > > > + (2) rhs3 vector has integer element type. > > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > + > > + if (TREE_CONSTANT (rhs3) > > + && VECTOR_TYPE_P (lhs_type) > > + && VECTOR_TYPE_P (rhs1_type) > > + && VECTOR_TYPE_P (rhs2_type) > > + && VECTOR_TYPE_P (rhs3_type) > > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) > > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) > > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) > > + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) > > + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) > > + return false; > > + > > I think this should be integrated into the existing conditions > rather than done as an initial special case. > > It might make sense to start with: > > if (TREE_CODE (rhs1_type) != VECTOR_TYPE > || TREE_CODE (rhs2_type) != VECTOR_TYPE > || TREE_CODE (rhs3_type) != VECTOR_TYPE) > { > > but expanded to test lhs_type too. Then the other parts of the new test > should be distributed across the existing conditions. > > The type tests should use useless_type_conversion_p rather than ==. Hi Richard, Thanks for the suggestions. In the attached patch, I tried to distribute the checks across existing conditions, does it look OK ? I am not sure how to write tests for the type checks tho, does gimple-fe support vec_perm_expr ? I did a quick testsuite run for vect.exp and the patch doesn't seem to cause any unexpected failures. Thanks, Prathamesh > > Thanks, > Richard > > > > > if (!useless_type_conversion_p (lhs_type, rhs1_type) > > || !useless_type_conversion_p (lhs_type, rhs2_type)) > > { [-- Attachment #2: pr96463-6-midend.txt --] [-- Type: text/plain, Size: 2264 bytes --] diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index e321d929fd0..a845c7fff93 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -4307,18 +4307,14 @@ verify_gimple_assign_ternary (gassign *stmt) break; case VEC_PERM_EXPR: - if (!useless_type_conversion_p (lhs_type, rhs1_type) - || !useless_type_conversion_p (lhs_type, rhs2_type)) - { - error ("type mismatch in %qs", code_name); - debug_generic_expr (lhs_type); - debug_generic_expr (rhs1_type); - debug_generic_expr (rhs2_type); - debug_generic_expr (rhs3_type); - return true; - } + /* If permute is constant, then we allow for lhs and rhs + to have different vector types, provided: + (1) lhs, rhs1, rhs2 have same element type. + (2) rhs3 vector has integer element type. + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ - if (TREE_CODE (rhs1_type) != VECTOR_TYPE + if (TREE_CODE (lhs_type) != VECTOR_TYPE + || TREE_CODE (rhs1_type) != VECTOR_TYPE || TREE_CODE (rhs2_type) != VECTOR_TYPE || TREE_CODE (rhs3_type) != VECTOR_TYPE) { @@ -4330,10 +4326,29 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } + /* If lhs, rhs1, and rhs2 are different vector types, + then relax the check if rhs3 is constant and lhs, rhs1, and rhs2 + have same element types. */ + if ((!useless_type_conversion_p (lhs_type, rhs1_type) + || !useless_type_conversion_p (lhs_type, rhs2_type)) + && (!TREE_CONSTANT (rhs3) + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs1_type) + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs2_type))) + { + error ("type mismatch in %qs", code_name); + debug_generic_expr (lhs_type); + debug_generic_expr (rhs1_type); + debug_generic_expr (rhs2_type); + debug_generic_expr (rhs3_type); + return true; + } + + /* If rhs3 is constant, relax the check len(rhs2) == len(rhs3). */ if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type)) - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), + || (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) + && !TREE_CONSTANT (rhs3)) || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), TYPE_VECTOR_SUBPARTS (lhs_type))) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-05-09 11:21 ` Prathamesh Kulkarni @ 2022-05-09 13:52 ` Richard Sandiford 2022-05-09 15:51 ` Prathamesh Kulkarni 0 siblings, 1 reply; 15+ messages in thread From: Richard Sandiford @ 2022-05-09 13:52 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Richard Biener, gcc Patches Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > On Tue, 3 May 2022 at 18:25, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> > On Tue, 4 Jan 2022 at 19:12, Richard Sandiford >> > <richard.sandiford@arm.com> wrote: >> >> >> >> Richard Biener <rguenther@suse.de> writes: >> >> > On Tue, 4 Jan 2022, Richard Sandiford wrote: >> >> > >> >> >> Richard Biener <rguenther@suse.de> writes: >> >> >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: >> >> >> > >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: >> >> >> >> > Hi, >> >> >> >> > The attached patch rearranges order of type-check for vec_perm_expr >> >> >> >> > and relaxes type checking for >> >> >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> >> >> >> >> > >> >> >> >> > when: >> >> >> >> > rhs1 == rhs2, >> >> >> >> > lhs is variable length vector, >> >> >> >> > rhs1 is fixed length vector, >> >> >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) >> >> >> >> > >> >> >> >> > I am not sure tho if this check is correct ? My intent was to capture >> >> >> >> > case when vec_perm_expr is used to "extend" fixed length vector to >> >> >> >> > it's VLA equivalent. >> >> >> >> >> >> >> >> VLAness isn't really the issue. We want the same thing to work for >> >> >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the >> >> >> >> vectors are fixed-length in that case. >> >> >> >> >> >> >> >> The principle is that for: >> >> >> >> >> >> >> >> A = VEC_PERM_EXPR <B, C, D>; >> >> >> >> >> >> >> >> the requirements are: >> >> >> >> >> >> >> >> - A, B, C and D must be vectors >> >> >> >> - A, B and C must have the same element type >> >> >> >> - D must have an integer element type >> >> >> >> - A and D must have the same number of elements (NA) >> >> >> >> - B and C must have the same number of elements (NB) >> >> >> >> >> >> >> >> The semantics are that we create a joined vector BC (all elements of B >> >> >> >> followed by all element of C) and that: >> >> >> >> >> >> >> >> A[i] = BC[D[i] % (NB+NB)] >> >> >> >> >> >> >> >> for 0 ≤ i < NA. >> >> >> >> >> >> >> >> This operation makes sense even if NA != NB. >> >> >> > >> >> >> > But note that we don't currently expect NA != NB and the optab just >> >> >> > has a single mode. >> >> >> >> >> >> True, but we only need this for constant permutes. They are already >> >> >> special in that they allow the index elements to be wider than the data >> >> >> elements. >> >> > >> >> > OK, then we should reflect this in the stmt verification and only relax >> >> > the constant permute vector case and also amend the >> >> > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. >> >> >> >> Sounds good. >> >> >> >> > For non-constant permutes the docs say the mode of vec_perm is >> >> > the common mode of operands 1 and 2 whilst the mode of operand 0 >> >> > is unspecified - even unconstrained by the docs. I'm not sure >> >> > if vec_perm expansion is expected to eventually FAIL. Updating the >> >> > docs of vec_perm would be appreciated as well. >> >> >> >> Yeah, I guess de facto operand 0 has to be the same mode as operands >> >> 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious >> >> or self-explanatory at the time. :-) >> >> >> >> > As said I prefer to not mangle the existing stmt checking too much >> >> > at this stage so minimal adjustment is prefered there. >> >> >> >> The PR is only an enhancement request rather than a bug, so I think the >> >> patch would need to wait for GCC 13 whatever happens. >> > Hi, >> > In attached patch, the type checking is relaxed only if mask is constant. >> > Does this look OK ? >> > >> > Thanks, >> > Prathamesh >> >> >> >> Thanks, >> >> Richard >> > >> > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc >> > index e321d929fd0..02b88f67855 100644 >> > --- a/gcc/tree-cfg.cc >> > +++ b/gcc/tree-cfg.cc >> > @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) >> > break; >> > >> > case VEC_PERM_EXPR: >> > + /* If permute is constant, then we allow for lhs and rhs >> > + to have different vector types, provided: >> > + (1) lhs, rhs1, rhs2, and rhs3 have same element type. >> >> This isn't a requirement for rhs3. >> >> > + (2) rhs3 vector has integer element type. >> > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ >> > + >> > + if (TREE_CONSTANT (rhs3) >> > + && VECTOR_TYPE_P (lhs_type) >> > + && VECTOR_TYPE_P (rhs1_type) >> > + && VECTOR_TYPE_P (rhs2_type) >> > + && VECTOR_TYPE_P (rhs3_type) >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) >> > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) >> > + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) >> > + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) >> > + return false; >> > + >> >> I think this should be integrated into the existing conditions >> rather than done as an initial special case. >> >> It might make sense to start with: >> >> if (TREE_CODE (rhs1_type) != VECTOR_TYPE >> || TREE_CODE (rhs2_type) != VECTOR_TYPE >> || TREE_CODE (rhs3_type) != VECTOR_TYPE) >> { >> >> but expanded to test lhs_type too. Then the other parts of the new test >> should be distributed across the existing conditions. >> >> The type tests should use useless_type_conversion_p rather than ==. > Hi Richard, > Thanks for the suggestions. In the attached patch, I tried to > distribute the checks > across existing conditions, does it look OK ? > I am not sure how to write tests for the type checks tho, does > gimple-fe support vec_perm_expr ? > I did a quick testsuite run for vect.exp and the patch doesn't seem to > cause any unexpected failures. > > Thanks, > Prathamesh >> >> Thanks, >> Richard >> >> >> >> > if (!useless_type_conversion_p (lhs_type, rhs1_type) >> > || !useless_type_conversion_p (lhs_type, rhs2_type)) >> > { > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > index e321d929fd0..a845c7fff93 100644 > --- a/gcc/tree-cfg.cc > +++ b/gcc/tree-cfg.cc > @@ -4307,18 +4307,14 @@ verify_gimple_assign_ternary (gassign *stmt) > break; > > case VEC_PERM_EXPR: > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > - { > - error ("type mismatch in %qs", code_name); > - debug_generic_expr (lhs_type); > - debug_generic_expr (rhs1_type); > - debug_generic_expr (rhs2_type); > - debug_generic_expr (rhs3_type); > - return true; > - } > + /* If permute is constant, then we allow for lhs and rhs > + to have different vector types, provided: > + (1) lhs, rhs1, rhs2 have same element type. > + (2) rhs3 vector has integer element type. > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > + if (TREE_CODE (lhs_type) != VECTOR_TYPE > + || TREE_CODE (rhs1_type) != VECTOR_TYPE > || TREE_CODE (rhs2_type) != VECTOR_TYPE > || TREE_CODE (rhs3_type) != VECTOR_TYPE) > { > @@ -4330,10 +4326,29 @@ verify_gimple_assign_ternary (gassign *stmt) > return true; > } > > + /* If lhs, rhs1, and rhs2 are different vector types, > + then relax the check if rhs3 is constant and lhs, rhs1, and rhs2 > + have same element types. */ > + if ((!useless_type_conversion_p (lhs_type, rhs1_type) > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > + && (!TREE_CONSTANT (rhs3) > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs1_type) > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs2_type))) These TREE_TYPE tests should use !useless_type_conversion_p too, instead of !=. Maybe it would be easier to follow as: if (TREE_CONSTANT (rhs3) ? ... : ...) so that we don't have doubled useless_type_conversion_p checks for the TREE_CONSTANT case. > + { > + error ("type mismatch in %qs", code_name); > + debug_generic_expr (lhs_type); > + debug_generic_expr (rhs1_type); > + debug_generic_expr (rhs2_type); > + debug_generic_expr (rhs3_type); > + return true; > + } > + > + /* If rhs3 is constant, relax the check len(rhs2) == len(rhs3). */ > if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > TYPE_VECTOR_SUBPARTS (rhs2_type)) > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > + || (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > TYPE_VECTOR_SUBPARTS (rhs3_type)) > + && !TREE_CONSTANT (rhs3)) Very minor, but I think this reads better with the !TREE_CONSTANT first in the && rather than second. There's no reason to compare the lengths for TREE_CONSTANT. Otherwise it looks good to me, but Richard should have the final say. Thanks, Richard > || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > TYPE_VECTOR_SUBPARTS (lhs_type))) > { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-05-09 13:52 ` Richard Sandiford @ 2022-05-09 15:51 ` Prathamesh Kulkarni 2022-05-23 17:27 ` Prathamesh Kulkarni 0 siblings, 1 reply; 15+ messages in thread From: Prathamesh Kulkarni @ 2022-05-09 15:51 UTC (permalink / raw) To: Prathamesh Kulkarni, Richard Biener, gcc Patches, richard.sandiford [-- Attachment #1: Type: text/plain, Size: 10101 bytes --] On Mon, 9 May 2022 at 19:22, Richard Sandiford <richard.sandiford@arm.com> wrote: > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > On Tue, 3 May 2022 at 18:25, Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> > On Tue, 4 Jan 2022 at 19:12, Richard Sandiford > >> > <richard.sandiford@arm.com> wrote: > >> >> > >> >> Richard Biener <rguenther@suse.de> writes: > >> >> > On Tue, 4 Jan 2022, Richard Sandiford wrote: > >> >> > > >> >> >> Richard Biener <rguenther@suse.de> writes: > >> >> >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: > >> >> >> > > >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > >> >> >> >> > Hi, > >> >> >> >> > The attached patch rearranges order of type-check for vec_perm_expr > >> >> >> >> > and relaxes type checking for > >> >> >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> > >> >> >> >> > > >> >> >> >> > when: > >> >> >> >> > rhs1 == rhs2, > >> >> >> >> > lhs is variable length vector, > >> >> >> >> > rhs1 is fixed length vector, > >> >> >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > >> >> >> >> > > >> >> >> >> > I am not sure tho if this check is correct ? My intent was to capture > >> >> >> >> > case when vec_perm_expr is used to "extend" fixed length vector to > >> >> >> >> > it's VLA equivalent. > >> >> >> >> > >> >> >> >> VLAness isn't really the issue. We want the same thing to work for > >> >> >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > >> >> >> >> vectors are fixed-length in that case. > >> >> >> >> > >> >> >> >> The principle is that for: > >> >> >> >> > >> >> >> >> A = VEC_PERM_EXPR <B, C, D>; > >> >> >> >> > >> >> >> >> the requirements are: > >> >> >> >> > >> >> >> >> - A, B, C and D must be vectors > >> >> >> >> - A, B and C must have the same element type > >> >> >> >> - D must have an integer element type > >> >> >> >> - A and D must have the same number of elements (NA) > >> >> >> >> - B and C must have the same number of elements (NB) > >> >> >> >> > >> >> >> >> The semantics are that we create a joined vector BC (all elements of B > >> >> >> >> followed by all element of C) and that: > >> >> >> >> > >> >> >> >> A[i] = BC[D[i] % (NB+NB)] > >> >> >> >> > >> >> >> >> for 0 ≤ i < NA. > >> >> >> >> > >> >> >> >> This operation makes sense even if NA != NB. > >> >> >> > > >> >> >> > But note that we don't currently expect NA != NB and the optab just > >> >> >> > has a single mode. > >> >> >> > >> >> >> True, but we only need this for constant permutes. They are already > >> >> >> special in that they allow the index elements to be wider than the data > >> >> >> elements. > >> >> > > >> >> > OK, then we should reflect this in the stmt verification and only relax > >> >> > the constant permute vector case and also amend the > >> >> > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. > >> >> > >> >> Sounds good. > >> >> > >> >> > For non-constant permutes the docs say the mode of vec_perm is > >> >> > the common mode of operands 1 and 2 whilst the mode of operand 0 > >> >> > is unspecified - even unconstrained by the docs. I'm not sure > >> >> > if vec_perm expansion is expected to eventually FAIL. Updating the > >> >> > docs of vec_perm would be appreciated as well. > >> >> > >> >> Yeah, I guess de facto operand 0 has to be the same mode as operands > >> >> 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious > >> >> or self-explanatory at the time. :-) > >> >> > >> >> > As said I prefer to not mangle the existing stmt checking too much > >> >> > at this stage so minimal adjustment is prefered there. > >> >> > >> >> The PR is only an enhancement request rather than a bug, so I think the > >> >> patch would need to wait for GCC 13 whatever happens. > >> > Hi, > >> > In attached patch, the type checking is relaxed only if mask is constant. > >> > Does this look OK ? > >> > > >> > Thanks, > >> > Prathamesh > >> >> > >> >> Thanks, > >> >> Richard > >> > > >> > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > >> > index e321d929fd0..02b88f67855 100644 > >> > --- a/gcc/tree-cfg.cc > >> > +++ b/gcc/tree-cfg.cc > >> > @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) > >> > break; > >> > > >> > case VEC_PERM_EXPR: > >> > + /* If permute is constant, then we allow for lhs and rhs > >> > + to have different vector types, provided: > >> > + (1) lhs, rhs1, rhs2, and rhs3 have same element type. > >> > >> This isn't a requirement for rhs3. > >> > >> > + (2) rhs3 vector has integer element type. > >> > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > >> > + > >> > + if (TREE_CONSTANT (rhs3) > >> > + && VECTOR_TYPE_P (lhs_type) > >> > + && VECTOR_TYPE_P (rhs1_type) > >> > + && VECTOR_TYPE_P (rhs2_type) > >> > + && VECTOR_TYPE_P (rhs3_type) > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) > >> > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) > >> > + return false; > >> > + > >> > >> I think this should be integrated into the existing conditions > >> rather than done as an initial special case. > >> > >> It might make sense to start with: > >> > >> if (TREE_CODE (rhs1_type) != VECTOR_TYPE > >> || TREE_CODE (rhs2_type) != VECTOR_TYPE > >> || TREE_CODE (rhs3_type) != VECTOR_TYPE) > >> { > >> > >> but expanded to test lhs_type too. Then the other parts of the new test > >> should be distributed across the existing conditions. > >> > >> The type tests should use useless_type_conversion_p rather than ==. > > Hi Richard, > > Thanks for the suggestions. In the attached patch, I tried to > > distribute the checks > > across existing conditions, does it look OK ? > > I am not sure how to write tests for the type checks tho, does > > gimple-fe support vec_perm_expr ? > > I did a quick testsuite run for vect.exp and the patch doesn't seem to > > cause any unexpected failures. > > > > Thanks, > > Prathamesh > >> > >> Thanks, > >> Richard > >> > >> > >> > >> > if (!useless_type_conversion_p (lhs_type, rhs1_type) > >> > || !useless_type_conversion_p (lhs_type, rhs2_type)) > >> > { > > > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > index e321d929fd0..a845c7fff93 100644 > > --- a/gcc/tree-cfg.cc > > +++ b/gcc/tree-cfg.cc > > @@ -4307,18 +4307,14 @@ verify_gimple_assign_ternary (gassign *stmt) > > break; > > > > case VEC_PERM_EXPR: > > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > > - { > > - error ("type mismatch in %qs", code_name); > > - debug_generic_expr (lhs_type); > > - debug_generic_expr (rhs1_type); > > - debug_generic_expr (rhs2_type); > > - debug_generic_expr (rhs3_type); > > - return true; > > - } > > + /* If permute is constant, then we allow for lhs and rhs > > + to have different vector types, provided: > > + (1) lhs, rhs1, rhs2 have same element type. > > + (2) rhs3 vector has integer element type. > > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > + if (TREE_CODE (lhs_type) != VECTOR_TYPE > > + || TREE_CODE (rhs1_type) != VECTOR_TYPE > > || TREE_CODE (rhs2_type) != VECTOR_TYPE > > || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > { > > @@ -4330,10 +4326,29 @@ verify_gimple_assign_ternary (gassign *stmt) > > return true; > > } > > > > + /* If lhs, rhs1, and rhs2 are different vector types, > > + then relax the check if rhs3 is constant and lhs, rhs1, and rhs2 > > + have same element types. */ > > + if ((!useless_type_conversion_p (lhs_type, rhs1_type) > > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > > + && (!TREE_CONSTANT (rhs3) > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs1_type) > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs2_type))) > > These TREE_TYPE tests should use !useless_type_conversion_p too, > instead of !=. Maybe it would be easier to follow as: > > if (TREE_CONSTANT (rhs3) > ? ... > : ...) > > so that we don't have doubled useless_type_conversion_p checks > for the TREE_CONSTANT case. > > > + { > > + error ("type mismatch in %qs", code_name); > > + debug_generic_expr (lhs_type); > > + debug_generic_expr (rhs1_type); > > + debug_generic_expr (rhs2_type); > > + debug_generic_expr (rhs3_type); > > + return true; > > + } > > + > > + /* If rhs3 is constant, relax the check len(rhs2) == len(rhs3). */ > > if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > TYPE_VECTOR_SUBPARTS (rhs2_type)) > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > + || (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > TYPE_VECTOR_SUBPARTS (rhs3_type)) > > + && !TREE_CONSTANT (rhs3)) > > Very minor, but I think this reads better with the !TREE_CONSTANT first > in the && rather than second. There's no reason to compare the lengths > for TREE_CONSTANT. > > Otherwise it looks good to me, but Richard should have the final say. Thanks, I addressed the above suggestions in the attached patch. Does it look OK ? Thanks, Prathamesh > > Thanks, > Richard > > > || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > > TYPE_VECTOR_SUBPARTS (lhs_type))) > > { [-- Attachment #2: pr96463-7-midend.txt --] [-- Type: text/plain, Size: 2341 bytes --] diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc index e321d929fd0..31f2514a407 100644 --- a/gcc/tree-cfg.cc +++ b/gcc/tree-cfg.cc @@ -4307,18 +4307,14 @@ verify_gimple_assign_ternary (gassign *stmt) break; case VEC_PERM_EXPR: - if (!useless_type_conversion_p (lhs_type, rhs1_type) - || !useless_type_conversion_p (lhs_type, rhs2_type)) - { - error ("type mismatch in %qs", code_name); - debug_generic_expr (lhs_type); - debug_generic_expr (rhs1_type); - debug_generic_expr (rhs2_type); - debug_generic_expr (rhs3_type); - return true; - } + /* If permute is constant, then we allow for lhs and rhs + to have different vector types, provided: + (1) lhs, rhs1, rhs2 have same element type. + (2) rhs3 vector has integer element type. + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ - if (TREE_CODE (rhs1_type) != VECTOR_TYPE + if (TREE_CODE (lhs_type) != VECTOR_TYPE + || TREE_CODE (rhs1_type) != VECTOR_TYPE || TREE_CODE (rhs2_type) != VECTOR_TYPE || TREE_CODE (rhs3_type) != VECTOR_TYPE) { @@ -4330,10 +4326,28 @@ verify_gimple_assign_ternary (gassign *stmt) return true; } + /* If rhs3 is constant, we allow lhs, rhs1 and rhs2 to be different vector types, + as long as lhs, rhs1 and rhs2 have same element type. */ + if (TREE_CONSTANT (rhs3) + ? (!useless_type_conversion_p (TREE_TYPE (lhs_type), TREE_TYPE (rhs1_type)) + || !useless_type_conversion_p (TREE_TYPE (lhs_type), TREE_TYPE (rhs2_type))) + : (!useless_type_conversion_p (lhs_type, rhs1_type) + || !useless_type_conversion_p (lhs_type, rhs2_type))) + { + error ("type mismatch in %qs", code_name); + debug_generic_expr (lhs_type); + debug_generic_expr (rhs1_type); + debug_generic_expr (rhs2_type); + debug_generic_expr (rhs3_type); + return true; + } + + /* If rhs3 is constant, relax the check len(rhs2) == len(rhs3). */ if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type)) - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), - TYPE_VECTOR_SUBPARTS (rhs3_type)) + || (!TREE_CONSTANT(rhs3) + && maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), + TYPE_VECTOR_SUBPARTS (rhs3_type))) || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), TYPE_VECTOR_SUBPARTS (lhs_type))) { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-05-09 15:51 ` Prathamesh Kulkarni @ 2022-05-23 17:27 ` Prathamesh Kulkarni 2022-05-31 11:35 ` Prathamesh Kulkarni 0 siblings, 1 reply; 15+ messages in thread From: Prathamesh Kulkarni @ 2022-05-23 17:27 UTC (permalink / raw) To: Prathamesh Kulkarni, Richard Biener, gcc Patches, richard.sandiford On Mon, 9 May 2022 at 21:21, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > > On Mon, 9 May 2022 at 19:22, Richard Sandiford > <richard.sandiford@arm.com> wrote: > > > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > On Tue, 3 May 2022 at 18:25, Richard Sandiford > > > <richard.sandiford@arm.com> wrote: > > >> > > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > >> > On Tue, 4 Jan 2022 at 19:12, Richard Sandiford > > >> > <richard.sandiford@arm.com> wrote: > > >> >> > > >> >> Richard Biener <rguenther@suse.de> writes: > > >> >> > On Tue, 4 Jan 2022, Richard Sandiford wrote: > > >> >> > > > >> >> >> Richard Biener <rguenther@suse.de> writes: > > >> >> >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: > > >> >> >> > > > >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > >> >> >> >> > Hi, > > >> >> >> >> > The attached patch rearranges order of type-check for vec_perm_expr > > >> >> >> >> > and relaxes type checking for > > >> >> >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> > > >> >> >> >> > > > >> >> >> >> > when: > > >> >> >> >> > rhs1 == rhs2, > > >> >> >> >> > lhs is variable length vector, > > >> >> >> >> > rhs1 is fixed length vector, > > >> >> >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > > >> >> >> >> > > > >> >> >> >> > I am not sure tho if this check is correct ? My intent was to capture > > >> >> >> >> > case when vec_perm_expr is used to "extend" fixed length vector to > > >> >> >> >> > it's VLA equivalent. > > >> >> >> >> > > >> >> >> >> VLAness isn't really the issue. We want the same thing to work for > > >> >> >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > > >> >> >> >> vectors are fixed-length in that case. > > >> >> >> >> > > >> >> >> >> The principle is that for: > > >> >> >> >> > > >> >> >> >> A = VEC_PERM_EXPR <B, C, D>; > > >> >> >> >> > > >> >> >> >> the requirements are: > > >> >> >> >> > > >> >> >> >> - A, B, C and D must be vectors > > >> >> >> >> - A, B and C must have the same element type > > >> >> >> >> - D must have an integer element type > > >> >> >> >> - A and D must have the same number of elements (NA) > > >> >> >> >> - B and C must have the same number of elements (NB) > > >> >> >> >> > > >> >> >> >> The semantics are that we create a joined vector BC (all elements of B > > >> >> >> >> followed by all element of C) and that: > > >> >> >> >> > > >> >> >> >> A[i] = BC[D[i] % (NB+NB)] > > >> >> >> >> > > >> >> >> >> for 0 ≤ i < NA. > > >> >> >> >> > > >> >> >> >> This operation makes sense even if NA != NB. > > >> >> >> > > > >> >> >> > But note that we don't currently expect NA != NB and the optab just > > >> >> >> > has a single mode. > > >> >> >> > > >> >> >> True, but we only need this for constant permutes. They are already > > >> >> >> special in that they allow the index elements to be wider than the data > > >> >> >> elements. > > >> >> > > > >> >> > OK, then we should reflect this in the stmt verification and only relax > > >> >> > the constant permute vector case and also amend the > > >> >> > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. > > >> >> > > >> >> Sounds good. > > >> >> > > >> >> > For non-constant permutes the docs say the mode of vec_perm is > > >> >> > the common mode of operands 1 and 2 whilst the mode of operand 0 > > >> >> > is unspecified - even unconstrained by the docs. I'm not sure > > >> >> > if vec_perm expansion is expected to eventually FAIL. Updating the > > >> >> > docs of vec_perm would be appreciated as well. > > >> >> > > >> >> Yeah, I guess de facto operand 0 has to be the same mode as operands > > >> >> 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious > > >> >> or self-explanatory at the time. :-) > > >> >> > > >> >> > As said I prefer to not mangle the existing stmt checking too much > > >> >> > at this stage so minimal adjustment is prefered there. > > >> >> > > >> >> The PR is only an enhancement request rather than a bug, so I think the > > >> >> patch would need to wait for GCC 13 whatever happens. > > >> > Hi, > > >> > In attached patch, the type checking is relaxed only if mask is constant. > > >> > Does this look OK ? > > >> > > > >> > Thanks, > > >> > Prathamesh > > >> >> > > >> >> Thanks, > > >> >> Richard > > >> > > > >> > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > >> > index e321d929fd0..02b88f67855 100644 > > >> > --- a/gcc/tree-cfg.cc > > >> > +++ b/gcc/tree-cfg.cc > > >> > @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) > > >> > break; > > >> > > > >> > case VEC_PERM_EXPR: > > >> > + /* If permute is constant, then we allow for lhs and rhs > > >> > + to have different vector types, provided: > > >> > + (1) lhs, rhs1, rhs2, and rhs3 have same element type. > > >> > > >> This isn't a requirement for rhs3. > > >> > > >> > + (2) rhs3 vector has integer element type. > > >> > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > >> > + > > >> > + if (TREE_CONSTANT (rhs3) > > >> > + && VECTOR_TYPE_P (lhs_type) > > >> > + && VECTOR_TYPE_P (rhs1_type) > > >> > + && VECTOR_TYPE_P (rhs2_type) > > >> > + && VECTOR_TYPE_P (rhs3_type) > > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) > > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) > > >> > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) > > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) > > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) > > >> > + return false; > > >> > + > > >> > > >> I think this should be integrated into the existing conditions > > >> rather than done as an initial special case. > > >> > > >> It might make sense to start with: > > >> > > >> if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > >> || TREE_CODE (rhs2_type) != VECTOR_TYPE > > >> || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > >> { > > >> > > >> but expanded to test lhs_type too. Then the other parts of the new test > > >> should be distributed across the existing conditions. > > >> > > >> The type tests should use useless_type_conversion_p rather than ==. > > > Hi Richard, > > > Thanks for the suggestions. In the attached patch, I tried to > > > distribute the checks > > > across existing conditions, does it look OK ? > > > I am not sure how to write tests for the type checks tho, does > > > gimple-fe support vec_perm_expr ? > > > I did a quick testsuite run for vect.exp and the patch doesn't seem to > > > cause any unexpected failures. > > > > > > Thanks, > > > Prathamesh > > >> > > >> Thanks, > > >> Richard > > >> > > >> > > >> > > >> > if (!useless_type_conversion_p (lhs_type, rhs1_type) > > >> > || !useless_type_conversion_p (lhs_type, rhs2_type)) > > >> > { > > > > > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > > index e321d929fd0..a845c7fff93 100644 > > > --- a/gcc/tree-cfg.cc > > > +++ b/gcc/tree-cfg.cc > > > @@ -4307,18 +4307,14 @@ verify_gimple_assign_ternary (gassign *stmt) > > > break; > > > > > > case VEC_PERM_EXPR: > > > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > > > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > - { > > > - error ("type mismatch in %qs", code_name); > > > - debug_generic_expr (lhs_type); > > > - debug_generic_expr (rhs1_type); > > > - debug_generic_expr (rhs2_type); > > > - debug_generic_expr (rhs3_type); > > > - return true; > > > - } > > > + /* If permute is constant, then we allow for lhs and rhs > > > + to have different vector types, provided: > > > + (1) lhs, rhs1, rhs2 have same element type. > > > + (2) rhs3 vector has integer element type. > > > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > > > > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > > + if (TREE_CODE (lhs_type) != VECTOR_TYPE > > > + || TREE_CODE (rhs1_type) != VECTOR_TYPE > > > || TREE_CODE (rhs2_type) != VECTOR_TYPE > > > || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > > { > > > @@ -4330,10 +4326,29 @@ verify_gimple_assign_ternary (gassign *stmt) > > > return true; > > > } > > > > > > + /* If lhs, rhs1, and rhs2 are different vector types, > > > + then relax the check if rhs3 is constant and lhs, rhs1, and rhs2 > > > + have same element types. */ > > > + if ((!useless_type_conversion_p (lhs_type, rhs1_type) > > > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > + && (!TREE_CONSTANT (rhs3) > > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs1_type) > > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs2_type))) > > > > These TREE_TYPE tests should use !useless_type_conversion_p too, > > instead of !=. Maybe it would be easier to follow as: > > > > if (TREE_CONSTANT (rhs3) > > ? ... > > : ...) > > > > so that we don't have doubled useless_type_conversion_p checks > > for the TREE_CONSTANT case. > > > > > + { > > > + error ("type mismatch in %qs", code_name); > > > + debug_generic_expr (lhs_type); > > > + debug_generic_expr (rhs1_type); > > > + debug_generic_expr (rhs2_type); > > > + debug_generic_expr (rhs3_type); > > > + return true; > > > + } > > > + > > > + /* If rhs3 is constant, relax the check len(rhs2) == len(rhs3). */ > > > if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > > TYPE_VECTOR_SUBPARTS (rhs2_type)) > > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > > + || (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > > TYPE_VECTOR_SUBPARTS (rhs3_type)) > > > + && !TREE_CONSTANT (rhs3)) > > > > Very minor, but I think this reads better with the !TREE_CONSTANT first > > in the && rather than second. There's no reason to compare the lengths > > for TREE_CONSTANT. > > > > Otherwise it looks good to me, but Richard should have the final say. > Thanks, I addressed the above suggestions in the attached patch. > Does it look OK ? ping https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594349.html Patch passes bootstrap+test on x86_64-linux-gnu. On aarch64-linux-gnu, the patch passes bootstrap, but I am seeing these discrepancies in test results: New tests that FAIL (2 tests): g++.dg/modules/macloc-1_c.C -std=c++17 dg-regexp 13 not found: "[^\\n]*macloc-1_c.C:8:7: error: too many arguments to function 'int you@agnes\\(\\)'\\nIn module agnes, imported at [^\\n]*macloc-1_b.C:8,\\nof module edith, imported at [^\\n]*macloc-1_c.C:3:\\n[^\\n]*macloc-1_a.C:12:14: note: declared here\\n[^\\n]*macloc-1_a.C:9:22: note: in definition of macro 'KEVIN'\\n" g++.dg/modules/macloc-1_c.C -std=c++17 (test for excess errors) Old tests that failed, that have disappeared (2 tests): (Eeek!) c-c++-common/missing-header-3.c -Wc++-compat expected multiline pattern lines 5-7 not found: "\\s*#import <this-file-does-not-exist\\.h>[^\\n\\r]*\\n \\^~~~~~~~~~~~~~~~~~~~~~~~~~~~\\ncompilation terminated\\.[^\\n\\r]*\\n" c-c++-common/missing-header-3.c -Wc++-compat (test for excess errors) I am not sure if these seem related to patch tho ? Thanks, Prathamesh > > Thanks, > Prathamesh > > > > Thanks, > > Richard > > > > > || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > > > TYPE_VECTOR_SUBPARTS (lhs_type))) > > > { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-05-23 17:27 ` Prathamesh Kulkarni @ 2022-05-31 11:35 ` Prathamesh Kulkarni 2022-06-01 7:58 ` Richard Biener 0 siblings, 1 reply; 15+ messages in thread From: Prathamesh Kulkarni @ 2022-05-31 11:35 UTC (permalink / raw) To: Prathamesh Kulkarni, Richard Biener, gcc Patches, richard.sandiford On Mon, 23 May 2022 at 22:57, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > > On Mon, 9 May 2022 at 21:21, Prathamesh Kulkarni > <prathamesh.kulkarni@linaro.org> wrote: > > > > On Mon, 9 May 2022 at 19:22, Richard Sandiford > > <richard.sandiford@arm.com> wrote: > > > > > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > > On Tue, 3 May 2022 at 18:25, Richard Sandiford > > > > <richard.sandiford@arm.com> wrote: > > > >> > > > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > >> > On Tue, 4 Jan 2022 at 19:12, Richard Sandiford > > > >> > <richard.sandiford@arm.com> wrote: > > > >> >> > > > >> >> Richard Biener <rguenther@suse.de> writes: > > > >> >> > On Tue, 4 Jan 2022, Richard Sandiford wrote: > > > >> >> > > > > >> >> >> Richard Biener <rguenther@suse.de> writes: > > > >> >> >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: > > > >> >> >> > > > > >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > >> >> >> >> > Hi, > > > >> >> >> >> > The attached patch rearranges order of type-check for vec_perm_expr > > > >> >> >> >> > and relaxes type checking for > > > >> >> >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> > > > >> >> >> >> > > > > >> >> >> >> > when: > > > >> >> >> >> > rhs1 == rhs2, > > > >> >> >> >> > lhs is variable length vector, > > > >> >> >> >> > rhs1 is fixed length vector, > > > >> >> >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > > > >> >> >> >> > > > > >> >> >> >> > I am not sure tho if this check is correct ? My intent was to capture > > > >> >> >> >> > case when vec_perm_expr is used to "extend" fixed length vector to > > > >> >> >> >> > it's VLA equivalent. > > > >> >> >> >> > > > >> >> >> >> VLAness isn't really the issue. We want the same thing to work for > > > >> >> >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > > > >> >> >> >> vectors are fixed-length in that case. > > > >> >> >> >> > > > >> >> >> >> The principle is that for: > > > >> >> >> >> > > > >> >> >> >> A = VEC_PERM_EXPR <B, C, D>; > > > >> >> >> >> > > > >> >> >> >> the requirements are: > > > >> >> >> >> > > > >> >> >> >> - A, B, C and D must be vectors > > > >> >> >> >> - A, B and C must have the same element type > > > >> >> >> >> - D must have an integer element type > > > >> >> >> >> - A and D must have the same number of elements (NA) > > > >> >> >> >> - B and C must have the same number of elements (NB) > > > >> >> >> >> > > > >> >> >> >> The semantics are that we create a joined vector BC (all elements of B > > > >> >> >> >> followed by all element of C) and that: > > > >> >> >> >> > > > >> >> >> >> A[i] = BC[D[i] % (NB+NB)] > > > >> >> >> >> > > > >> >> >> >> for 0 ≤ i < NA. > > > >> >> >> >> > > > >> >> >> >> This operation makes sense even if NA != NB. > > > >> >> >> > > > > >> >> >> > But note that we don't currently expect NA != NB and the optab just > > > >> >> >> > has a single mode. > > > >> >> >> > > > >> >> >> True, but we only need this for constant permutes. They are already > > > >> >> >> special in that they allow the index elements to be wider than the data > > > >> >> >> elements. > > > >> >> > > > > >> >> > OK, then we should reflect this in the stmt verification and only relax > > > >> >> > the constant permute vector case and also amend the > > > >> >> > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. > > > >> >> > > > >> >> Sounds good. > > > >> >> > > > >> >> > For non-constant permutes the docs say the mode of vec_perm is > > > >> >> > the common mode of operands 1 and 2 whilst the mode of operand 0 > > > >> >> > is unspecified - even unconstrained by the docs. I'm not sure > > > >> >> > if vec_perm expansion is expected to eventually FAIL. Updating the > > > >> >> > docs of vec_perm would be appreciated as well. > > > >> >> > > > >> >> Yeah, I guess de facto operand 0 has to be the same mode as operands > > > >> >> 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious > > > >> >> or self-explanatory at the time. :-) > > > >> >> > > > >> >> > As said I prefer to not mangle the existing stmt checking too much > > > >> >> > at this stage so minimal adjustment is prefered there. > > > >> >> > > > >> >> The PR is only an enhancement request rather than a bug, so I think the > > > >> >> patch would need to wait for GCC 13 whatever happens. > > > >> > Hi, > > > >> > In attached patch, the type checking is relaxed only if mask is constant. > > > >> > Does this look OK ? > > > >> > > > > >> > Thanks, > > > >> > Prathamesh > > > >> >> > > > >> >> Thanks, > > > >> >> Richard > > > >> > > > > >> > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > > >> > index e321d929fd0..02b88f67855 100644 > > > >> > --- a/gcc/tree-cfg.cc > > > >> > +++ b/gcc/tree-cfg.cc > > > >> > @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) > > > >> > break; > > > >> > > > > >> > case VEC_PERM_EXPR: > > > >> > + /* If permute is constant, then we allow for lhs and rhs > > > >> > + to have different vector types, provided: > > > >> > + (1) lhs, rhs1, rhs2, and rhs3 have same element type. > > > >> > > > >> This isn't a requirement for rhs3. > > > >> > > > >> > + (2) rhs3 vector has integer element type. > > > >> > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > > >> > + > > > >> > + if (TREE_CONSTANT (rhs3) > > > >> > + && VECTOR_TYPE_P (lhs_type) > > > >> > + && VECTOR_TYPE_P (rhs1_type) > > > >> > + && VECTOR_TYPE_P (rhs2_type) > > > >> > + && VECTOR_TYPE_P (rhs3_type) > > > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) > > > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) > > > >> > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) > > > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) > > > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) > > > >> > + return false; > > > >> > + > > > >> > > > >> I think this should be integrated into the existing conditions > > > >> rather than done as an initial special case. > > > >> > > > >> It might make sense to start with: > > > >> > > > >> if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > > >> || TREE_CODE (rhs2_type) != VECTOR_TYPE > > > >> || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > > >> { > > > >> > > > >> but expanded to test lhs_type too. Then the other parts of the new test > > > >> should be distributed across the existing conditions. > > > >> > > > >> The type tests should use useless_type_conversion_p rather than ==. > > > > Hi Richard, > > > > Thanks for the suggestions. In the attached patch, I tried to > > > > distribute the checks > > > > across existing conditions, does it look OK ? > > > > I am not sure how to write tests for the type checks tho, does > > > > gimple-fe support vec_perm_expr ? > > > > I did a quick testsuite run for vect.exp and the patch doesn't seem to > > > > cause any unexpected failures. > > > > > > > > Thanks, > > > > Prathamesh > > > >> > > > >> Thanks, > > > >> Richard > > > >> > > > >> > > > >> > > > >> > if (!useless_type_conversion_p (lhs_type, rhs1_type) > > > >> > || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > >> > { > > > > > > > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > > > index e321d929fd0..a845c7fff93 100644 > > > > --- a/gcc/tree-cfg.cc > > > > +++ b/gcc/tree-cfg.cc > > > > @@ -4307,18 +4307,14 @@ verify_gimple_assign_ternary (gassign *stmt) > > > > break; > > > > > > > > case VEC_PERM_EXPR: > > > > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > > > > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > > - { > > > > - error ("type mismatch in %qs", code_name); > > > > - debug_generic_expr (lhs_type); > > > > - debug_generic_expr (rhs1_type); > > > > - debug_generic_expr (rhs2_type); > > > > - debug_generic_expr (rhs3_type); > > > > - return true; > > > > - } > > > > + /* If permute is constant, then we allow for lhs and rhs > > > > + to have different vector types, provided: > > > > + (1) lhs, rhs1, rhs2 have same element type. > > > > + (2) rhs3 vector has integer element type. > > > > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > > > > > > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > > > + if (TREE_CODE (lhs_type) != VECTOR_TYPE > > > > + || TREE_CODE (rhs1_type) != VECTOR_TYPE > > > > || TREE_CODE (rhs2_type) != VECTOR_TYPE > > > > || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > > > { > > > > @@ -4330,10 +4326,29 @@ verify_gimple_assign_ternary (gassign *stmt) > > > > return true; > > > > } > > > > > > > > + /* If lhs, rhs1, and rhs2 are different vector types, > > > > + then relax the check if rhs3 is constant and lhs, rhs1, and rhs2 > > > > + have same element types. */ > > > > + if ((!useless_type_conversion_p (lhs_type, rhs1_type) > > > > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > > + && (!TREE_CONSTANT (rhs3) > > > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs1_type) > > > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs2_type))) > > > > > > These TREE_TYPE tests should use !useless_type_conversion_p too, > > > instead of !=. Maybe it would be easier to follow as: > > > > > > if (TREE_CONSTANT (rhs3) > > > ? ... > > > : ...) > > > > > > so that we don't have doubled useless_type_conversion_p checks > > > for the TREE_CONSTANT case. > > > > > > > + { > > > > + error ("type mismatch in %qs", code_name); > > > > + debug_generic_expr (lhs_type); > > > > + debug_generic_expr (rhs1_type); > > > > + debug_generic_expr (rhs2_type); > > > > + debug_generic_expr (rhs3_type); > > > > + return true; > > > > + } > > > > + > > > > + /* If rhs3 is constant, relax the check len(rhs2) == len(rhs3). */ > > > > if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > > > TYPE_VECTOR_SUBPARTS (rhs2_type)) > > > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > > > + || (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > > > TYPE_VECTOR_SUBPARTS (rhs3_type)) > > > > + && !TREE_CONSTANT (rhs3)) > > > > > > Very minor, but I think this reads better with the !TREE_CONSTANT first > > > in the && rather than second. There's no reason to compare the lengths > > > for TREE_CONSTANT. > > > > > > Otherwise it looks good to me, but Richard should have the final say. > > Thanks, I addressed the above suggestions in the attached patch. > > Does it look OK ? > ping https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594349.html > Patch passes bootstrap+test on x86_64-linux-gnu. > On aarch64-linux-gnu, the patch passes bootstrap, but I am seeing > these discrepancies in test results: > > New tests that FAIL (2 tests): > > g++.dg/modules/macloc-1_c.C -std=c++17 dg-regexp 13 not found: > "[^\\n]*macloc-1_c.C:8:7: error: too many arguments to function 'int > you@agnes\\(\\)'\\nIn module agnes, imported at > [^\\n]*macloc-1_b.C:8,\\nof module edith, imported at > [^\\n]*macloc-1_c.C:3:\\n[^\\n]*macloc-1_a.C:12:14: note: declared > here\\n[^\\n]*macloc-1_a.C:9:22: note: in definition of macro > 'KEVIN'\\n" > g++.dg/modules/macloc-1_c.C -std=c++17 (test for excess errors) > > Old tests that failed, that have disappeared (2 tests): (Eeek!) > > c-c++-common/missing-header-3.c -Wc++-compat expected multiline > pattern lines 5-7 not found: "\\s*#import > <this-file-does-not-exist\\.h>[^\\n\\r]*\\n > \\^~~~~~~~~~~~~~~~~~~~~~~~~~~~\\ncompilation > terminated\\.[^\\n\\r]*\\n" > c-c++-common/missing-header-3.c -Wc++-compat (test for excess errors) > > I am not sure if these seem related to patch tho ? ping * 2 https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594349.html Thanks, Prathamesh > > Thanks, > Prathamesh > > > > Thanks, > > Prathamesh > > > > > > Thanks, > > > Richard > > > > > > > || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs3_type), > > > > TYPE_VECTOR_SUBPARTS (lhs_type))) > > > > { ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end 2022-05-31 11:35 ` Prathamesh Kulkarni @ 2022-06-01 7:58 ` Richard Biener 0 siblings, 0 replies; 15+ messages in thread From: Richard Biener @ 2022-06-01 7:58 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, richard.sandiford On Tue, 31 May 2022, Prathamesh Kulkarni wrote: > On Mon, 23 May 2022 at 22:57, Prathamesh Kulkarni > <prathamesh.kulkarni@linaro.org> wrote: > > > > On Mon, 9 May 2022 at 21:21, Prathamesh Kulkarni > > <prathamesh.kulkarni@linaro.org> wrote: > > > > > > On Mon, 9 May 2022 at 19:22, Richard Sandiford > > > <richard.sandiford@arm.com> wrote: > > > > > > > > Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > > > On Tue, 3 May 2022 at 18:25, Richard Sandiford > > > > > <richard.sandiford@arm.com> wrote: > > > > >> > > > > >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > > >> > On Tue, 4 Jan 2022 at 19:12, Richard Sandiford > > > > >> > <richard.sandiford@arm.com> wrote: > > > > >> >> > > > > >> >> Richard Biener <rguenther@suse.de> writes: > > > > >> >> > On Tue, 4 Jan 2022, Richard Sandiford wrote: > > > > >> >> > > > > > >> >> >> Richard Biener <rguenther@suse.de> writes: > > > > >> >> >> > On Fri, 17 Dec 2021, Richard Sandiford wrote: > > > > >> >> >> > > > > > >> >> >> >> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes: > > > > >> >> >> >> > Hi, > > > > >> >> >> >> > The attached patch rearranges order of type-check for vec_perm_expr > > > > >> >> >> >> > and relaxes type checking for > > > > >> >> >> >> > lhs = vec_perm_expr<rhs1, rhs2, mask> > > > > >> >> >> >> > > > > > >> >> >> >> > when: > > > > >> >> >> >> > rhs1 == rhs2, > > > > >> >> >> >> > lhs is variable length vector, > > > > >> >> >> >> > rhs1 is fixed length vector, > > > > >> >> >> >> > TREE_TYPE (lhs) == TREE_TYPE (rhs1) > > > > >> >> >> >> > > > > > >> >> >> >> > I am not sure tho if this check is correct ? My intent was to capture > > > > >> >> >> >> > case when vec_perm_expr is used to "extend" fixed length vector to > > > > >> >> >> >> > it's VLA equivalent. > > > > >> >> >> >> > > > > >> >> >> >> VLAness isn't really the issue. We want the same thing to work for > > > > >> >> >> >> -msve-vector-bits=256, -msve-vector-bits=512, etc., even though the > > > > >> >> >> >> vectors are fixed-length in that case. > > > > >> >> >> >> > > > > >> >> >> >> The principle is that for: > > > > >> >> >> >> > > > > >> >> >> >> A = VEC_PERM_EXPR <B, C, D>; > > > > >> >> >> >> > > > > >> >> >> >> the requirements are: > > > > >> >> >> >> > > > > >> >> >> >> - A, B, C and D must be vectors > > > > >> >> >> >> - A, B and C must have the same element type > > > > >> >> >> >> - D must have an integer element type > > > > >> >> >> >> - A and D must have the same number of elements (NA) > > > > >> >> >> >> - B and C must have the same number of elements (NB) > > > > >> >> >> >> > > > > >> >> >> >> The semantics are that we create a joined vector BC (all elements of B > > > > >> >> >> >> followed by all element of C) and that: > > > > >> >> >> >> > > > > >> >> >> >> A[i] = BC[D[i] % (NB+NB)] > > > > >> >> >> >> > > > > >> >> >> >> for 0 ? i < NA. > > > > >> >> >> >> > > > > >> >> >> >> This operation makes sense even if NA != NB. > > > > >> >> >> > > > > > >> >> >> > But note that we don't currently expect NA != NB and the optab just > > > > >> >> >> > has a single mode. > > > > >> >> >> > > > > >> >> >> True, but we only need this for constant permutes. They are already > > > > >> >> >> special in that they allow the index elements to be wider than the data > > > > >> >> >> elements. > > > > >> >> > > > > > >> >> > OK, then we should reflect this in the stmt verification and only relax > > > > >> >> > the constant permute vector case and also amend the > > > > >> >> > TARGET_VECTORIZE_VEC_PERM_CONST accordingly. > > > > >> >> > > > > >> >> Sounds good. > > > > >> >> > > > > >> >> > For non-constant permutes the docs say the mode of vec_perm is > > > > >> >> > the common mode of operands 1 and 2 whilst the mode of operand 0 > > > > >> >> > is unspecified - even unconstrained by the docs. I'm not sure > > > > >> >> > if vec_perm expansion is expected to eventually FAIL. Updating the > > > > >> >> > docs of vec_perm would be appreciated as well. > > > > >> >> > > > > >> >> Yeah, I guess de facto operand 0 has to be the same mode as operands > > > > >> >> 1 and 2. Maybe that was just an oversight, or maybe it seemed obvious > > > > >> >> or self-explanatory at the time. :-) > > > > >> >> > > > > >> >> > As said I prefer to not mangle the existing stmt checking too much > > > > >> >> > at this stage so minimal adjustment is prefered there. > > > > >> >> > > > > >> >> The PR is only an enhancement request rather than a bug, so I think the > > > > >> >> patch would need to wait for GCC 13 whatever happens. > > > > >> > Hi, > > > > >> > In attached patch, the type checking is relaxed only if mask is constant. > > > > >> > Does this look OK ? > > > > >> > > > > > >> > Thanks, > > > > >> > Prathamesh > > > > >> >> > > > > >> >> Thanks, > > > > >> >> Richard > > > > >> > > > > > >> > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > > > >> > index e321d929fd0..02b88f67855 100644 > > > > >> > --- a/gcc/tree-cfg.cc > > > > >> > +++ b/gcc/tree-cfg.cc > > > > >> > @@ -4307,6 +4307,24 @@ verify_gimple_assign_ternary (gassign *stmt) > > > > >> > break; > > > > >> > > > > > >> > case VEC_PERM_EXPR: > > > > >> > + /* If permute is constant, then we allow for lhs and rhs > > > > >> > + to have different vector types, provided: > > > > >> > + (1) lhs, rhs1, rhs2, and rhs3 have same element type. > > > > >> > > > > >> This isn't a requirement for rhs3. > > > > >> > > > > >> > + (2) rhs3 vector has integer element type. > > > > >> > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > > > >> > + > > > > >> > + if (TREE_CONSTANT (rhs3) > > > > >> > + && VECTOR_TYPE_P (lhs_type) > > > > >> > + && VECTOR_TYPE_P (rhs1_type) > > > > >> > + && VECTOR_TYPE_P (rhs2_type) > > > > >> > + && VECTOR_TYPE_P (rhs3_type) > > > > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs1_type) > > > > >> > + && TREE_TYPE (lhs_type) == TREE_TYPE (rhs2_type) > > > > >> > + && INTEGRAL_TYPE_P (TREE_TYPE (rhs3_type)) > > > > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (lhs_type), TYPE_VECTOR_SUBPARTS (rhs3_type)) > > > > >> > + && known_eq (TYPE_VECTOR_SUBPARTS (rhs1_type), TYPE_VECTOR_SUBPARTS (rhs2_type))) > > > > >> > + return false; > > > > >> > + > > > > >> > > > > >> I think this should be integrated into the existing conditions > > > > >> rather than done as an initial special case. > > > > >> > > > > >> It might make sense to start with: > > > > >> > > > > >> if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > > > >> || TREE_CODE (rhs2_type) != VECTOR_TYPE > > > > >> || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > > > >> { > > > > >> > > > > >> but expanded to test lhs_type too. Then the other parts of the new test > > > > >> should be distributed across the existing conditions. > > > > >> > > > > >> The type tests should use useless_type_conversion_p rather than ==. > > > > > Hi Richard, > > > > > Thanks for the suggestions. In the attached patch, I tried to > > > > > distribute the checks > > > > > across existing conditions, does it look OK ? > > > > > I am not sure how to write tests for the type checks tho, does > > > > > gimple-fe support vec_perm_expr ? > > > > > I did a quick testsuite run for vect.exp and the patch doesn't seem to > > > > > cause any unexpected failures. > > > > > > > > > > Thanks, > > > > > Prathamesh > > > > >> > > > > >> Thanks, > > > > >> Richard > > > > >> > > > > >> > > > > >> > > > > >> > if (!useless_type_conversion_p (lhs_type, rhs1_type) > > > > >> > || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > > >> > { > > > > > > > > > > diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc > > > > > index e321d929fd0..a845c7fff93 100644 > > > > > --- a/gcc/tree-cfg.cc > > > > > +++ b/gcc/tree-cfg.cc > > > > > @@ -4307,18 +4307,14 @@ verify_gimple_assign_ternary (gassign *stmt) > > > > > break; > > > > > > > > > > case VEC_PERM_EXPR: > > > > > - if (!useless_type_conversion_p (lhs_type, rhs1_type) > > > > > - || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > > > - { > > > > > - error ("type mismatch in %qs", code_name); > > > > > - debug_generic_expr (lhs_type); > > > > > - debug_generic_expr (rhs1_type); > > > > > - debug_generic_expr (rhs2_type); > > > > > - debug_generic_expr (rhs3_type); > > > > > - return true; > > > > > - } > > > > > + /* If permute is constant, then we allow for lhs and rhs > > > > > + to have different vector types, provided: > > > > > + (1) lhs, rhs1, rhs2 have same element type. > > > > > + (2) rhs3 vector has integer element type. > > > > > + (3) len(lhs) == len(rhs3) && len(rhs1) == len(rhs2). */ > > > > > > > > > > - if (TREE_CODE (rhs1_type) != VECTOR_TYPE > > > > > + if (TREE_CODE (lhs_type) != VECTOR_TYPE > > > > > + || TREE_CODE (rhs1_type) != VECTOR_TYPE > > > > > || TREE_CODE (rhs2_type) != VECTOR_TYPE > > > > > || TREE_CODE (rhs3_type) != VECTOR_TYPE) > > > > > { > > > > > @@ -4330,10 +4326,29 @@ verify_gimple_assign_ternary (gassign *stmt) > > > > > return true; > > > > > } > > > > > > > > > > + /* If lhs, rhs1, and rhs2 are different vector types, > > > > > + then relax the check if rhs3 is constant and lhs, rhs1, and rhs2 > > > > > + have same element types. */ > > > > > + if ((!useless_type_conversion_p (lhs_type, rhs1_type) > > > > > + || !useless_type_conversion_p (lhs_type, rhs2_type)) > > > > > + && (!TREE_CONSTANT (rhs3) > > > > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs1_type) > > > > > + || TREE_TYPE (lhs_type) != TREE_TYPE (rhs2_type))) > > > > > > > > These TREE_TYPE tests should use !useless_type_conversion_p too, > > > > instead of !=. Maybe it would be easier to follow as: > > > > > > > > if (TREE_CONSTANT (rhs3) > > > > ? ... > > > > : ...) > > > > > > > > so that we don't have doubled useless_type_conversion_p checks > > > > for the TREE_CONSTANT case. > > > > > > > > > + { > > > > > + error ("type mismatch in %qs", code_name); > > > > > + debug_generic_expr (lhs_type); > > > > > + debug_generic_expr (rhs1_type); > > > > > + debug_generic_expr (rhs2_type); > > > > > + debug_generic_expr (rhs3_type); > > > > > + return true; > > > > > + } > > > > > + > > > > > + /* If rhs3 is constant, relax the check len(rhs2) == len(rhs3). */ > > > > > if (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs1_type), > > > > > TYPE_VECTOR_SUBPARTS (rhs2_type)) > > > > > - || maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > > > > + || (maybe_ne (TYPE_VECTOR_SUBPARTS (rhs2_type), > > > > > TYPE_VECTOR_SUBPARTS (rhs3_type)) > > > > > + && !TREE_CONSTANT (rhs3)) > > > > > > > > Very minor, but I think this reads better with the !TREE_CONSTANT first > > > > in the && rather than second. There's no reason to compare the lengths > > > > for TREE_CONSTANT. > > > > > > > > Otherwise it looks good to me, but Richard should have the final say. > > > Thanks, I addressed the above suggestions in the attached patch. > > > Does it look OK ? > > ping https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594349.html > > Patch passes bootstrap+test on x86_64-linux-gnu. > > On aarch64-linux-gnu, the patch passes bootstrap, but I am seeing > > these discrepancies in test results: > > > > New tests that FAIL (2 tests): > > > > g++.dg/modules/macloc-1_c.C -std=c++17 dg-regexp 13 not found: > > "[^\\n]*macloc-1_c.C:8:7: error: too many arguments to function 'int > > you@agnes\\(\\)'\\nIn module agnes, imported at > > [^\\n]*macloc-1_b.C:8,\\nof module edith, imported at > > [^\\n]*macloc-1_c.C:3:\\n[^\\n]*macloc-1_a.C:12:14: note: declared > > here\\n[^\\n]*macloc-1_a.C:9:22: note: in definition of macro > > 'KEVIN'\\n" > > g++.dg/modules/macloc-1_c.C -std=c++17 (test for excess errors) > > > > Old tests that failed, that have disappeared (2 tests): (Eeek!) > > > > c-c++-common/missing-header-3.c -Wc++-compat expected multiline > > pattern lines 5-7 not found: "\\s*#import > > <this-file-does-not-exist\\.h>[^\\n\\r]*\\n > > \\^~~~~~~~~~~~~~~~~~~~~~~~~~~~\\ncompilation > > terminated\\.[^\\n\\r]*\\n" > > c-c++-common/missing-header-3.c -Wc++-compat (test for excess errors) > > > > I am not sure if these seem related to patch tho ? > ping * 2 https://gcc.gnu.org/pipermail/gcc-patches/2022-May/594349.html OK. Richard. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-06-01 7:58 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-17 10:05 [2/2] PR96463 -- changes to type checking vec_perm_expr in middle end Prathamesh Kulkarni 2021-12-17 11:07 ` Richard Sandiford 2021-12-27 10:25 ` Prathamesh Kulkarni 2022-01-03 10:40 ` Richard Biener 2022-01-04 11:50 ` Richard Sandiford 2022-01-04 12:40 ` Richard Biener 2022-01-04 13:42 ` Richard Sandiford 2022-05-03 10:41 ` Prathamesh Kulkarni 2022-05-03 12:55 ` Richard Sandiford 2022-05-09 11:21 ` Prathamesh Kulkarni 2022-05-09 13:52 ` Richard Sandiford 2022-05-09 15:51 ` Prathamesh Kulkarni 2022-05-23 17:27 ` Prathamesh Kulkarni 2022-05-31 11:35 ` Prathamesh Kulkarni 2022-06-01 7:58 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).