On Thu, Jul 6, 2023 at 6:18 PM Xi Ruoyao via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > If a bit-field is signed and it's wider than the output type, we must > ensure the extracted result sign-extended. But this was not handled > correctly. > > For example: > > int x : 8; > long y : 55; > bool z : 1; > > The vectorized extraction of y was: > > vect__ifc__49.29_110 = > MEM <vector(2) long unsigned int> [(struct Item *)vectp_a.27_108]; > vect_patt_38.30_112 = > vect__ifc__49.29_110 & { 9223372036854775552, 9223372036854775552 }; > vect_patt_39.31_113 = vect_patt_38.30_112 >> 8; > vect_patt_40.32_114 = > VIEW_CONVERT_EXPR<vector(2) long int>(vect_patt_39.31_113); > > This is obviously incorrect. This pach has implemented it as: > > vect__ifc__25.16_62 = > MEM <vector(2) long unsigned int> [(struct Item *)vectp_a.14_60]; > vect_patt_31.17_63 = > VIEW_CONVERT_EXPR<vector(2) long int>(vect__ifc__25.16_62); > vect_patt_32.18_64 = vect_patt_31.17_63 << 1; > vect_patt_33.19_65 = vect_patt_32.18_64 >> 9; > > gcc/ChangeLog: > > PR tree-optimization/110557 > * tree-vect-patterns.cc (vect_recog_bitfield_ref_pattern): > Ensure the output sign-extended if necessary. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/110557 > * g++.dg/vect/pr110557.cc: New test. > --- > > Bootstrapped and regtested on x86_64-linux-gnu. Ok for trunk and gcc-13 > branch? > > gcc/testsuite/g++.dg/vect/pr110557.cc | 37 +++++++++++++++++ > gcc/tree-vect-patterns.cc | 58 ++++++++++++++++++++------- > 2 files changed, 81 insertions(+), 14 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/vect/pr110557.cc > > diff --git a/gcc/testsuite/g++.dg/vect/pr110557.cc > b/gcc/testsuite/g++.dg/vect/pr110557.cc > new file mode 100644 > index 00000000000..e1fbe1caac4 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/vect/pr110557.cc > @@ -0,0 +1,37 @@ > +// { dg-additional-options "-mavx" { target { avx_runtime } } } > + > +static inline long > +min (long a, long b) > +{ > + return a < b ? a : b; > +} > + > +struct Item > +{ > + int x : 8; > + long y : 55; > + bool z : 1; > +}; > + > +__attribute__ ((noipa)) long > +test (Item *a, int cnt) > +{ > + long size = 0; > + for (int i = 0; i < cnt; i++) > + size = min ((long)a[i].y, size); > + return size; > +} > + > +int > +main () > +{ > + struct Item items[] = { > + { 1, -1 }, > + { 2, -2 }, > + { 3, -3 }, > + { 4, -4 }, > + }; > + > + if (test (items, 4) != -4) > + __builtin_trap (); > +} > diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc > index 1bc36b043a0..20412c27ead 100644 > --- a/gcc/tree-vect-patterns.cc > +++ b/gcc/tree-vect-patterns.cc > @@ -2566,7 +2566,7 @@ vect_recog_widen_sum_pattern (vec_info *vinfo, > Widening with mask first, shift later: > container = (type_out) container; > masked = container & (((1 << bitsize) - 1) << bitpos); > - result = patt2 >> masked; > + result = masked >> bitpos; > > Widening with shift first, mask last: > container = (type_out) container; > @@ -2578,6 +2578,15 @@ vect_recog_widen_sum_pattern (vec_info *vinfo, > result = masked >> bitpos; > result = (type_out) result; > > + If the bitfield is signed and it's wider than type_out, we need to > + keep the result sign-extended: > + container = (type) container; > + masked = container << (prec - bitsize - bitpos); > + result = (type_out) (masked >> (prec - bitsize)); > + > + Here type is the signed variant of the wider of type_out and the type > + of container. > + > The shifting is always optional depending on whether bitpos != 0. > > */ > @@ -2636,14 +2645,22 @@ vect_recog_bitfield_ref_pattern (vec_info *vinfo, > stmt_vec_info stmt_info, > if (BYTES_BIG_ENDIAN) > shift_n = prec - shift_n - mask_width; > > + bool sign_ext = (!TYPE_UNSIGNED (TREE_TYPE (bf_ref)) && > + TYPE_PRECISION (ret_type) > mask_width); > + bool widening = ((TYPE_PRECISION (TREE_TYPE (container)) < > + TYPE_PRECISION (ret_type)) > + && !useless_type_conversion_p (TREE_TYPE (container), > + ret_type));
the !useless_type_conversion_p check isn't necessary, when TYPE_PRECISION isn't equal the conversion is never useless. I'll also note that ret_type == TREE_TYPE (bf_ref). Can you rename 'widening' to 'load_widen' and 'sign_ext' to 'ref_sext'? As they are named it suggest they apply to the same so I originally thought sign_ext should be widening && !TYPE_UNSIGNED. Otherwise looks reasonable. Thanks, Richard. > + > /* We move the conversion earlier if the loaded type is smaller than the > return type to enable the use of widening loads. */ > - if (TYPE_PRECISION (TREE_TYPE (container)) < TYPE_PRECISION (ret_type) > - && !useless_type_conversion_p (TREE_TYPE (container), ret_type)) > + if (sign_ext || widening) > { > - pattern_stmt > - = gimple_build_assign (vect_recog_temp_ssa_var (ret_type), > - NOP_EXPR, container); > + tree type = widening ? ret_type : container_type; > + if (sign_ext) > + type = gimple_signed_type (type); > + pattern_stmt = gimple_build_assign (vect_recog_temp_ssa_var (type), > + NOP_EXPR, container); > container = gimple_get_lhs (pattern_stmt); > container_type = TREE_TYPE (container); > prec = tree_to_uhwi (TYPE_SIZE (container_type)); > @@ -2671,7 +2688,7 @@ vect_recog_bitfield_ref_pattern (vec_info *vinfo, > stmt_vec_info stmt_info, > shift_first = true; > > tree result; > - if (shift_first) > + if (shift_first && !sign_ext) > { > tree shifted = container; > if (shift_n) > @@ -2694,14 +2711,27 @@ vect_recog_bitfield_ref_pattern (vec_info *vinfo, > stmt_vec_info stmt_info, > } > else > { > - tree mask = wide_int_to_tree (container_type, > - wi::shifted_mask (shift_n, mask_width, > - false, prec)); > - pattern_stmt > - = gimple_build_assign (vect_recog_temp_ssa_var (container_type), > - BIT_AND_EXPR, container, mask); > - tree masked = gimple_assign_lhs (pattern_stmt); > + tree temp = vect_recog_temp_ssa_var (container_type); > + if (!sign_ext) > + { > + tree mask = wide_int_to_tree (container_type, > + wi::shifted_mask (shift_n, > + mask_width, > + false, prec)); > + pattern_stmt = gimple_build_assign (temp, BIT_AND_EXPR, > + container, mask); > + } > + else > + { > + HOST_WIDE_INT shl = prec - shift_n - mask_width; > + shift_n += shl; > + pattern_stmt = gimple_build_assign (temp, LSHIFT_EXPR, > + container, > + build_int_cst (sizetype, > + shl)); > + } > > + tree masked = gimple_assign_lhs (pattern_stmt); > append_pattern_def_seq (vinfo, stmt_info, pattern_stmt, vectype); > pattern_stmt > = gimple_build_assign (vect_recog_temp_ssa_var (container_type), > -- > 2.41.0 >