From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x534.google.com (mail-ed1-x534.google.com [IPv6:2a00:1450:4864:20::534]) by sourceware.org (Postfix) with ESMTPS id CCC5B385828E for ; Thu, 21 Jul 2022 06:51:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org CCC5B385828E Received: by mail-ed1-x534.google.com with SMTP id x91so991120ede.1 for ; Wed, 20 Jul 2022 23:51:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=CWzerG170H6+ksiKFb1GLtf2HRSW625WmoYexbCR+qk=; b=6tZzLtouNd6xDsZNNW87bAW6GsAi0aWlQRcAgjyWqqDeLHLtBFq9TGYQSF9EzhltKF G7FjDSdhToDQIsCPAH9670z/E1laljvY0p65EpW6UVytFhUQLvXr76fhA/XR/IUrsiLw Dwn9M8VrVlS0DNetkf9VdKKhwtuDeMkfDxZSK0psPdHQt8NI1SRq/fd6tk2Kh1pHMmEx AgT8MOP8d6rUzboMwK8NqSvTf4nBmhecUGvKudhbDxqxcvnoeNHpKtERn9tGPabB8sj4 21qvYlIdO/7Qf9dGznAp0/81lQgGmMsErW6tK6Ge/CSda42DJ9LeEKSx2j6zLDEOHAI2 hazg== X-Gm-Message-State: AJIora+c09cLNWhJDd8A+iTSKjlaTi7LV0SQ66F1VPrs/JWGQDe5Ykkx NOxvoPo77wGZoEZkbPppR5rjCqJ46ryUh3EJCcKgOFTpmkY= X-Google-Smtp-Source: AGRyM1sZ+1quGQ2NyBLa8Mg+nwQoVytbJxv3ax6udrbOcq5hg7mT0VMAXYebVOfJDTfBvJPrcRTtt4Yk8eDCWuo8HFw= X-Received: by 2002:a05:6402:4488:b0:43a:7b6e:4b04 with SMTP id er8-20020a056402448800b0043a7b6e4b04mr54853920edb.202.1658386302338; Wed, 20 Jul 2022 23:51:42 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Thu, 21 Jul 2022 08:51:29 +0200 Message-ID: Subject: Re: ICE after folding svld1rq to vec_perm_expr duing forwprop To: Prathamesh Kulkarni Cc: gcc Patches , Richard Sandiford Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, WEIRD_PORT autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jul 2022 06:51:46 -0000 On Wed, Jul 20, 2022 at 5:36 PM Prathamesh Kulkarni wrote: > > On Mon, 18 Jul 2022 at 11:57, Richard Biener = wrote: > > > > On Fri, Jul 15, 2022 at 3:49 PM Prathamesh Kulkarni > > wrote: > > > > > > On Thu, 14 Jul 2022 at 17:22, Richard Sandiford > > > wrote: > > > > > > > > Richard Biener writes: > > > > > On Thu, Jul 14, 2022 at 9:55 AM Prathamesh Kulkarni > > > > > wrote: > > > > >> > > > > >> On Wed, 13 Jul 2022 at 12:22, Richard Biener wrote: > > > > >> > > > > > >> > On Tue, Jul 12, 2022 at 9:12 PM Prathamesh Kulkarni via Gcc-pa= tches > > > > >> > wrote: > > > > >> > > > > > > >> > > Hi Richard, > > > > >> > > For the following test: > > > > >> > > > > > > >> > > svint32_t f2(int a, int b, int c, int d) > > > > >> > > { > > > > >> > > int32x4_t v =3D (int32x4_t) {a, b, c, d}; > > > > >> > > return svld1rq_s32 (svptrue_b8 (), &v[0]); > > > > >> > > } > > > > >> > > > > > > >> > > The compiler emits following ICE with -O3 -mcpu=3Dgeneric+sv= e: > > > > >> > > foo.c: In function =E2=80=98f2=E2=80=99: > > > > >> > > foo.c:4:11: error: non-trivial conversion in =E2=80=98view_c= onvert_expr=E2=80=99 > > > > >> > > 4 | svint32_t f2(int a, int b, int c, int d) > > > > >> > > | ^~ > > > > >> > > svint32_t > > > > >> > > __Int32x4_t > > > > >> > > _7 =3D 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; > > > > >> > > > > > > >> > > : > > > > >> > > _1 =3D {a_3(D), b_4(D), c_5(D), d_6(D)}; > > > > >> > > v_12 =3D _1; > > > > >> > > _11 =3D v_12; > > > > >> > > _9 =3D VEC_PERM_EXPR <_11, _11, { 0, 1, 2, 3, ... }>; > > > > >> > > return _9; > > > > >> > > > > > > >> > > During forwprop, simplify_permutation simplifies vec_perm_ex= pr to > > > > >> > > view_convert_expr, > > > > >> > > and the end result becomes: > > > > >> > > svint32_t _7; > > > > >> > > __Int32x4_t _8; > > > > >> > > > > > > >> > > ;; basic block 2, loop depth 0 > > > > >> > > ;; pred: ENTRY > > > > >> > > _8 =3D {a_2(D), b_3(D), c_4(D), d_5(D)}; > > > > >> > > _7 =3D VIEW_CONVERT_EXPR<__Int32x4_t>(_8); > > > > >> > > return _7; > > > > >> > > ;; succ: EXIT > > > > >> > > > > > > >> > > which causes the error duing verify_gimple since VIEW_CONVER= T_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 appr= oach ? > > > > >> > > > > > >> > 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 t= o > > > > >> 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 =3D (int32x4_t) {1, 2, 3, 4}; > > > > >> svint32_t v2 =3D svld1rq_s32 (svptrue_b8 (), &v[0]); > > > > >> return v2; > > > > >> } > > > > >> > > > > >> foo2.c: In function =E2=80=98foo=E2=80=99: > > > > >> foo2.c:9:1: error: non-trivial conversion in =E2=80=98vector_cst= =E2=80=99 > > > > >> 9 | } > > > > >> | ^ > > > > >> svint32_t > > > > >> int32x4_t > > > > >> v2_4 =3D { 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 d= on'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 tryin= g > > > > to build a 128-bit vector from 2 64-bit vectors. And there are oth= er > > > > 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_E= XPR > > > > of type T needs to be replaced by something of type T. Whether T h= as 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 permu= ting > > > > > a VLA vector and then refuse to go any of the non-VEC_PERM genera= ting > > > > > paths - that probably means just allowing the code =3D=3D VEC_PER= M_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)) !=3D TREE_TYPE (type) > > > > || TREE_TYPE (TREE_TYPE (arg1)) !=3D 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 wher= e the > > > > outputs are a different length from the inputs, since the all-const= ant > > > > 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 =3D false; > > > tree res_type =3D TREE_TYPE (arg0); > > > tree opt =3D fold_ternary (VEC_PERM_EXPR, res_type, arg0, arg1,= op2); > > > > > > Using, res_type =3D TREE_TYPE (gimple_get_lhs (stmt)), > > > resolves the ICE, and emits all constant VEC_PERM_EXPR: > > > > > > v2_4 =3D 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 =3D (void *) ivtmp.21_73; > _92 =3D MEM [(uint8_t *)_78]; > _91 =3D {_92, 0}; > vect__1.6_90 =3D VIEW_CONVERT_EXPR(_91); > _88 =3D MEM [(uint8_t *)_78 + 16B]; > _87 =3D {_88, 0}; > vect__1.7_86 =3D VIEW_CONVERT_EXPR(_87); > vect__1.8_85 =3D VEC_PERM_EXPR 3, 4, 5, 6, 7, 16, 17, 18, 19, 20, 21, 22, 23 }>; > > So for, > tree res_type =3D TREE_TYPE (gimple_assign_lhs (stmt)); > tree opt =3D fold_ternary (VEC_PERM_EXPR, res_type, arg0, arg1, op2); > > we have: > res_type =3D V16QI > arg0 =3D {_92, 0} > arg1 =3D {_88, 0} > op2 =3D {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 =3D=3D 2, and TYPE_VECTOR_SUBPARTS (type) =3D=3D 16. > > If we revert the committed patch so we pass res_type =3D TREE_TYPE (arg0)= instead, > it simplifies the above VEC_PERM_EXPR to VIEW_CONVERT_EXPR: > _92 =3D MEM [(uint8_t *)_78]; > _88 =3D MEM [(uint8_t *)_78 + 16B]; > _5 =3D {_92, _88}; > vect__1.8_85 =3D VIEW_CONVERT_EXPR(_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 =3D { (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 chec= ks > if lhs_type and res_type differ but have same size, and in that case emit= : > lhs =3D VIEW_CONVERT_EXPR (opt), > instead of: > lhs =3D VIEW_CONVERT_EXPR (opt) > where opt is result of folding VEC_PERM_EXPR > > Does it look OK ? Definitely the original change was bogus. + if (!operand_equal_p (TYPE_SIZE (lhs_type), TYPE_SIZE (res_type))= ) + return 0; just repeats your very original change though ... I'll note that fold_ternary will ICE on now valid VEC_PERM_EXPRs so we should fix it, possibly by returning NULL_TREE on cases it does not handle. I think what should be done is, in the /* If there are any VIEW_CONVERT_EXPRs found when finding permutation operands source, check whether it's valid to transform and prepare the required new operands. */ if (code =3D=3D VIEW_CONVERT_EXPR || code2 =3D=3D VIEW_CONVERT_EXPR) { ... path also transform the expected result type. It should remain V_C_E compa= tible to TREE_TYPE (lhs) but get a new element type. But as said, tree fold_vec_perm (tree type, tree arg0, tree arg1, const vec_perm_indices &sel= ) { unsigned int i; unsigned HOST_WIDE_INT nelts; bool need_ctor =3D false; if (!sel.length ().is_constant (&nelts)) return NULL_TREE; 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))= ; ^^^ this doesn't match what we allow for VEC_PERM_EXPRs now and fold_ternar= y doesn't guard according to those asserts (I think we should extend fold_vec= _perm to support the new constraints). Richard. > Thanks, > Prathamesh > > > > > > I will try to address the folding for above VEC_PERM_EXPR in follow-u= p patch. > > > > > > Thanks, > > > Prathamesh > > > > > > > > Thanks, > > > > Richard