Re: ICE after folding svld1rq to vec_perm_expr duing forwprop

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 10081 bytes --]

On Mon, 18 Jul 2022 at 11:57, Richard Biener <richard.guenther@gmail.com> wrote:
>
> On Fri, Jul 15, 2022 at 3:49 PM Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
> >
> > On Thu, 14 Jul 2022 at 17:22, Richard Sandiford
> > <richard.sandiford@arm.com> wrote:
> > >
> > > Richard Biener <richard.guenther@gmail.com> writes:
> > > > On Thu, Jul 14, 2022 at 9:55 AM Prathamesh Kulkarni
> > > > <prathamesh.kulkarni@linaro.org> wrote:
> > > >>
> > > >> On Wed, 13 Jul 2022 at 12:22, Richard Biener <richard.guenther@gmail.com> wrote:
> > > >> >
> > > >> > On Tue, Jul 12, 2022 at 9:12 PM Prathamesh Kulkarni via Gcc-patches
> > > >> > <gcc-patches@gcc.gnu.org> wrote:
> > > >> > >
> > > >> > > Hi Richard,
> > > >> > > For the following test:
> > > >> > >
> > > >> > > svint32_t f2(int a, int b, int c, int d)
> > > >> > > {
> > > >> > >   int32x4_t v = (int32x4_t) {a, b, c, d};
> > > >> > >   return svld1rq_s32 (svptrue_b8 (), &v[0]);
> > > >> > > }
> > > >> > >
> > > >> > > The compiler emits following ICE with -O3 -mcpu=generic+sve:
> > > >> > > foo.c: In function ‘f2’:
> > > >> > > foo.c:4:11: error: non-trivial conversion in ‘view_convert_expr’
> > > >> > >     4 | svint32_t f2(int a, int b, int c, int d)
> > > >> > >       |           ^~
> > > >> > > svint32_t
> > > >> > > __Int32x4_t
> > > >> > > _7 = VIEW_CONVERT_EXPR<__Int32x4_t>(_8);
> > > >> > > during GIMPLE pass: forwprop
> > > >> > > dump file: foo.c.109t.forwprop2
> > > >> > > foo.c:4:11: internal compiler error: verify_gimple failed
> > > >> > > 0xfda04a verify_gimple_in_cfg(function*, bool)
> > > >> > >         ../../gcc/gcc/tree-cfg.cc:5568
> > > >> > > 0xe9371f execute_function_todo
> > > >> > >         ../../gcc/gcc/passes.cc:2091
> > > >> > > 0xe93ccb execute_todo
> > > >> > >         ../../gcc/gcc/passes.cc:2145
> > > >> > >
> > > >> > > This happens because, after folding svld1rq_s32 to vec_perm_expr, we have:
> > > >> > >   int32x4_t v;
> > > >> > >   __Int32x4_t _1;
> > > >> > >   svint32_t _9;
> > > >> > >   vector(4) int _11;
> > > >> > >
> > > >> > >   <bb 2> :
> > > >> > >   _1 = {a_3(D), b_4(D), c_5(D), d_6(D)};
> > > >> > >   v_12 = _1;
> > > >> > >   _11 = v_12;
> > > >> > >   _9 = VEC_PERM_EXPR <_11, _11, { 0, 1, 2, 3, ... }>;
> > > >> > >   return _9;
> > > >> > >
> > > >> > > During forwprop, simplify_permutation simplifies vec_perm_expr to
> > > >> > > view_convert_expr,
> > > >> > > and the end result becomes:
> > > >> > >   svint32_t _7;
> > > >> > >   __Int32x4_t _8;
> > > >> > >
> > > >> > > ;;   basic block 2, loop depth 0
> > > >> > > ;;    pred:       ENTRY
> > > >> > >   _8 = {a_2(D), b_3(D), c_4(D), d_5(D)};
> > > >> > >   _7 = VIEW_CONVERT_EXPR<__Int32x4_t>(_8);
> > > >> > >   return _7;
> > > >> > > ;;    succ:       EXIT
> > > >> > >
> > > >> > > which causes the error duing verify_gimple since VIEW_CONVERT_EXPR
> > > >> > > has incompatible types (svint32_t, int32x4_t).
> > > >> > >
> > > >> > > The attached patch disables simplification of VEC_PERM_EXPR
> > > >> > > in simplify_permutation, if lhs and rhs have non compatible types,
> > > >> > > which resolves ICE, but am not sure if it's the correct approach ?
> > > >> >
> > > >> > It for sure papers over the issue.  I think the error happens earlier,
> > > >> > the V_C_E should have been built with the type of the VEC_PERM_EXPR
> > > >> > which is the type of the LHS.  But then you probably run into the
> > > >> > different sizes ICE (VLA vs constant size).  I think for this case you
> > > >> > want a BIT_FIELD_REF instead of a VIEW_CONVERT_EXPR,
> > > >> > selecting the "low" part of the VLA vector.
> > > >> Hi Richard,
> > > >> Sorry I don't quite follow. In this case, we use VEC_PERM_EXPR to
> > > >> represent dup operation
> > > >> from fixed width to VLA vector. I am not sure how folding it to
> > > >> BIT_FIELD_REF will work.
> > > >> Could you please elaborate ?
> > > >>
> > > >> Also, the issue doesn't seem restricted to this case.
> > > >> The following test case also ICE's during forwprop:
> > > >> svint32_t foo()
> > > >> {
> > > >>   int32x4_t v = (int32x4_t) {1, 2, 3, 4};
> > > >>   svint32_t v2 = svld1rq_s32 (svptrue_b8 (), &v[0]);
> > > >>   return v2;
> > > >> }
> > > >>
> > > >> foo2.c: In function ‘foo’:
> > > >> foo2.c:9:1: error: non-trivial conversion in ‘vector_cst’
> > > >>     9 | }
> > > >>       | ^
> > > >> svint32_t
> > > >> int32x4_t
> > > >> v2_4 = { 1, 2, 3, 4 };
> > > >>
> > > >> because simplify_permutation folds
> > > >> VEC_PERM_EXPR< {1, 2, 3, 4}, {1, 2, 3, 4}, {0, 1, 2, 3, ...} >
> > > >> into:
> > > >> vector_cst {1, 2, 3, 4}
> > > >>
> > > >> and it complains during verify_gimple_assign_single because we don't
> > > >> support assignment of vector_cst to VLA vector.
> > > >>
> > > >> I guess the issue really is that currently, only VEC_PERM_EXPR
> > > >> supports lhs and rhs
> > > >> to have vector types with differing lengths, and simplifying it to
> > > >> other tree codes, like above,
> > > >> will result in type errors ?
> > > >
> > > > That might be the case - Richard should know.
> > >
> > > I don't see anything particularly special about VEC_PERM_EXPR here,
> > > or about the VLA vs. VLS thing.  We would have the same issue trying
> > > to build a 128-bit vector from 2 64-bit vectors.  And there are other
> > > tree codes whose input types are/can be different from their output
> > > types.
> > >
> > > So it just seems like a normal type correctness issue: a VEC_PERM_EXPR
> > > of type T needs to be replaced by something of type T.  Whether T has a
> > > constant size or a variable size doesn't matter.
> > >
> > > > If so your type check
> > > > is still too late, you should instead recognize that we are permuting
> > > > a VLA vector and then refuse to go any of the non-VEC_PERM generating
> > > > paths - that probably means just allowing the code == VEC_PERM_EXPR
> > > > case and not any of the CTOR/CST/VIEW_CONVERT_EXPR cases?
> > >
> > > Yeah.  If we're talking about the match.pd code, I think only:
> > >
> > >   (if (sel.series_p (0, 1, 0, 1))
> > >    { op0; }
> > >    (if (sel.series_p (0, 1, nelts, 1))
> > >     { op1; }
> > >
> > > need a type compatibility check.  For fold_vec_perm I think
> > > we should just rearrange:
> > >
> > >   gcc_assert (known_eq (TYPE_VECTOR_SUBPARTS (type), nelts)
> > >               && known_eq (TYPE_VECTOR_SUBPARTS (TREE_TYPE (arg0)), nelts)
> > >               && known_eq (TYPE_VECTOR_SUBPARTS (TREE_TYPE (arg1)), nelts));
> > >   if (TREE_TYPE (TREE_TYPE (arg0)) != TREE_TYPE (type)
> > >       || TREE_TYPE (TREE_TYPE (arg1)) != TREE_TYPE (type))
> > >     return NULL_TREE;
> > >
> > > so that the assert comes after the early-out.
> > >
> > > It would be good at some point to relax fold_vec_perm to cases where the
> > > outputs are a different length from the inputs, since the all-constant
> > > VEC_PERM_EXPR above could be folded to a VECTOR_CST.
> > Hi,
> > For the above case, I think the issue is that simplify_permutation
> > uses TREE_TYPE (arg0) for res_type,
> > while it should now use type for lhs.
> >
> >       /* Shuffle of a constructor.  */
> >       bool ret = false;
> >       tree res_type = TREE_TYPE (arg0);
> >       tree opt = fold_ternary (VEC_PERM_EXPR, res_type, arg0, arg1, op2);
> >
> > Using, res_type = TREE_TYPE (gimple_get_lhs (stmt)),
> > resolves the ICE, and emits all constant VEC_PERM_EXPR:
> >
> >   v2_4 = VEC_PERM_EXPR <{ 1, 2, 3, 4 }, { 1, 2, 3, 4 }, { 0, 1, 2, 3, ... }>;
> >   return v2_4;
> >
> > Does the patch look OK to commit after bootstrap+test ?
>
> Ok with using gimple_assign_lhs (stmt) instead of gimple_get_lhs (stmt).
Hi,
I committed the patch but unfortunately it caused PR106360.
The issue is that for slp-reduc-sad-2.c on ppc64le,
simplify_permutation sees the following during forwprop4:

  _78 = (void *) ivtmp.21_73;
  _92 = MEM <unsigned long> [(uint8_t *)_78];
  _91 = {_92, 0};
  vect__1.6_90 = VIEW_CONVERT_EXPR<vector(16) unsigned char>(_91);
  _88 = MEM <unsigned long> [(uint8_t *)_78 + 16B];
  _87 = {_88, 0};
  vect__1.7_86 = VIEW_CONVERT_EXPR<vector(16) unsigned char>(_87);
  vect__1.8_85 = VEC_PERM_EXPR <vect__1.6_90, vect__1.7_86, { 0, 1, 2,
3, 4, 5, 6, 7, 16, 17, 18, 19, 20, 21, 22, 23 }>;

So for,
tree res_type = TREE_TYPE (gimple_assign_lhs (stmt));
tree opt = fold_ternary (VEC_PERM_EXPR, res_type, arg0, arg1, op2);

we have:
res_type = V16QI
arg0 = {_92, 0}
arg1 = {_88, 0}
op2 = {0, 2}

and thus we hit the following assert in fold_vec_perm:

gcc_assert (known_eq (TYPE_VECTOR_SUBPARTS (type), nelts)
              && known_eq (TYPE_VECTOR_SUBPARTS (TREE_TYPE (arg0)), nelts)
              && known_eq (TYPE_VECTOR_SUBPARTS (TREE_TYPE (arg1)), nelts));

since nelts == 2, and TYPE_VECTOR_SUBPARTS (type) == 16.

If we revert the committed patch so we pass res_type = TREE_TYPE (arg0) instead,
it simplifies the above VEC_PERM_EXPR to VIEW_CONVERT_EXPR:
  _92 = MEM <unsigned long> [(uint8_t *)_78];
  _88 = MEM <unsigned long> [(uint8_t *)_78 + 16B];
  _5 = {_92, _88};
  vect__1.8_85 = VIEW_CONVERT_EXPR<vector(16) unsigned char>(_5);

I suppose it's legal to cast vector of one type to another as long as
sizes match ?

IIUC, the above VIEW_CONVERT_EXPR will result in:
vect__1.8_85 = { (uint8_t) _92, 0, 0, 0, 0, 0, 0, 0, (uint8_t) _88, 0,
0, 0, 0, 0, 0, 0 } ?

In the attached patch, it restores res_type to TREE_TYPE (arg0), and checks
if lhs_type and res_type differ but have same size, and in that case emit:
lhs = VIEW_CONVERT_EXPR<lhs_type> (opt),
instead of:
lhs = VIEW_CONVERT_EXPR<op1 type> (opt)
where opt is result of folding VEC_PERM_EXPR<res_type, arg0, arg1, op2>

Does it look OK ?

Thanks,
Prathamesh

>
> > I will try to address the folding for above VEC_PERM_EXPR in follow-up patch.
> >
> > Thanks,
> > Prathamesh
> > >
> > > Thanks,
> > > Richard

[-- Attachment #2: pr106360-1.txt --]
[-- Type: text/plain, Size: 1252 bytes --]

diff --git a/gcc/tree-ssa-forwprop.cc b/gcc/tree-ssa-forwprop.cc
index d04cf4bccf8..6019d9b03ff 100644
--- a/gcc/tree-ssa-forwprop.cc
+++ b/gcc/tree-ssa-forwprop.cc
@@ -2661,18 +2661,21 @@ simplify_permutation (gimple_stmt_iterator *gsi)
 
       /* Shuffle of a constructor.  */
       bool ret = false;
-      tree res_type = TREE_TYPE (gimple_assign_lhs (stmt));
+      tree res_type = TREE_TYPE (arg0);
       tree opt = fold_ternary (VEC_PERM_EXPR, res_type, arg0, arg1, op2);
       if (!opt
 	  || (TREE_CODE (opt) != CONSTRUCTOR && TREE_CODE (opt) != VECTOR_CST))
 	return 0;
       /* Found VIEW_CONVERT_EXPR before, need one explicit conversion.  */
-      if (res_type != TREE_TYPE (op0))
+      tree lhs_type = TREE_TYPE (gimple_assign_lhs (stmt));
+      if (res_type != lhs_type)
 	{
+	  if (!operand_equal_p (TYPE_SIZE (lhs_type), TYPE_SIZE (res_type)))
+	    return 0;
 	  tree name = make_ssa_name (TREE_TYPE (opt));
 	  gimple *ass_stmt = gimple_build_assign (name, opt);
 	  gsi_insert_before (gsi, ass_stmt, GSI_SAME_STMT);
-	  opt = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (op0), name);
+	  opt = build1 (VIEW_CONVERT_EXPR, lhs_type, name);
 	}
       gimple_assign_set_rhs_from_tree (gsi, opt);
       update_stmt (gsi_stmt (*gsi));