From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by sourceware.org (Postfix) with ESMTPS id 3B5DA3858D32 for ; Wed, 15 Nov 2023 15:14:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 3B5DA3858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 3B5DA3858D32 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::436 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700061291; cv=none; b=xqoK4PoYUOCedFH0O4sTDl0YegTIgN9EI7xI9NYGWbiif20QLRU3HKc3B9/MmZdDWn3gN88qs8OFv5hhWTuTA9vw9AybBw5h1q8UKO6a3CvhvgXkbdE/g0VYp/t5Z+STTHcNsUHzWwBr55GKnJakoslYXUUgQS70hBUpyV4fuN0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1700061291; c=relaxed/simple; bh=jkRok/H4eKIEeRt6Pc0/65xLXxr3hmK/IVKTheeBsGU=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=cGtZTcV4DFhZkUctPiyjD4DLheKXIHJtAs/doq5pE9iaXhyzkVFeBNnLrxMEpKFi5rpYhJU/U4gCFN20YQ0Q6BbcrpqJbPNOYa8DbqH+SzqgmmRxT8CPmNkb3i9zHSPmk7cyuQVUDkbjCat9dSjll67ZLdjmYlisSPAWMRaBpAI= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-wr1-x436.google.com with SMTP id ffacd0b85a97d-32d9d8284abso4411594f8f.3 for ; Wed, 15 Nov 2023 07:14:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1700061286; x=1700666086; darn=gcc.gnu.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=ZYkv0gRH/OYmOVLn8OHFsCM1QjJeafwW6cMhmXB2+50=; b=IpiNncFu09A3tjd5f6uWQayZ9LIzTSeAM34KMG+1Ev/GrOq2cP5geLxmHLIwerQRvp e7MtDtLiML6Kx92QMsY6hRa0hAL5l0OTPX40XV470GPdT9auB8SW7c2USmd8ySE62hFQ DPIugJgBSpvjE79376Xhl2c6NmiXEvdJ7YpFb8351oGnHhqydFiEq8pOdRGSixiCLIUx a6N5qh2b0EgR5hcZKunJciz/IkW7+2VKy3mAghmpatiaywxXbEK158hdMfBRLm7pw7am SMmmEd0MrBNJ88X9aK1YZ17rGkg7eBbH8MOugEDzeVOBUej05kYb9MPyS3+E12VJ83bW 3e+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1700061286; x=1700666086; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=ZYkv0gRH/OYmOVLn8OHFsCM1QjJeafwW6cMhmXB2+50=; b=f9k2jOiFQyx5J5D7gYPRKWAvkeiRrhjkD9SNpfPIk2bdiyKVC36ZwUY+8VgPFl5vFR dA+kony9fddENWjfotws3BtpOTBDLp1Cs6Z62BHPhpSovbgHzu+7QMVH2nl12KJETU83 plEne1bskFNoG0fo8r59DeGWRrqBcK1pSmhZsYG/SkNoJvWazCl1yO47xYE6D5+WfiYw MRyoG7B7+sEOnW8FNNpKEt6Bri5jHqEK/3p+ZUNc0D1ue6Rqf5j1zu1uQrhrDbThEX5B QUtEyyZELb+bxVOeQ/eVGcoo0fGvUzQ/nFHsXaw+wpJfulTYu/HRedePAusqHmgPsKDn Zqtw== X-Gm-Message-State: AOJu0Yzt0l7ZClT7SCsMJ0C0jWZauELTAdZTeti/HEcImvDy84gKOD04 weLn7XK14LRpHNzvBNEOMCWRzfs9EwARDsuTVmx3mQ== X-Google-Smtp-Source: AGHT+IHCnX490ZQZl6daJaxstGDjQ2UYb4x+re+NFCqG8VcR0iM3vWl1oGTYaUIMgF6l2st5qVYHpCexx50ZSpSVEjo= X-Received: by 2002:a5d:6d04:0:b0:32d:906f:b423 with SMTP id e4-20020a5d6d04000000b0032d906fb423mr11015524wrq.67.1700061285602; Wed, 15 Nov 2023 07:14:45 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Prathamesh Kulkarni Date: Wed, 15 Nov 2023 20:44:09 +0530 Message-ID: Subject: Re: PR111754 To: Prathamesh Kulkarni , gcc Patches , Richard Biener , richard.sandiford@arm.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_NUMSUBJECT,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,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 List-Id: On Wed, 8 Nov 2023 at 21:57, Prathamesh Kulkarni wrote: > > On Thu, 26 Oct 2023 at 09:43, Prathamesh Kulkarni > wrote: > > > > On Thu, 26 Oct 2023 at 04:09, Richard Sandiford > > wrote: > > > > > > Prathamesh Kulkarni writes: > > > > On Wed, 25 Oct 2023 at 02:58, Richard Sandiford > > > > wrote: > > > >> > > > >> Hi, > > > >> > > > >> Sorry the slow review. I clearly didn't think this through proper= ly > > > >> when doing the review of the original patch, so I wanted to spend > > > >> some time working on the code to get a better understanding of > > > >> the problem. > > > >> > > > >> Prathamesh Kulkarni writes: > > > >> > Hi, > > > >> > For the following test-case: > > > >> > > > > >> > typedef float __attribute__((__vector_size__ (16))) F; > > > >> > F foo (F a, F b) > > > >> > { > > > >> > F v =3D (F) { 9 }; > > > >> > return __builtin_shufflevector (v, v, 1, 0, 1, 2); > > > >> > } > > > >> > > > > >> > Compiling with -O2 results in following ICE: > > > >> > foo.c: In function =E2=80=98foo=E2=80=99: > > > >> > foo.c:6:10: internal compiler error: in decompose, at rtl.h:2314 > > > >> > 6 | return __builtin_shufflevector (v, v, 1, 0, 1, 2); > > > >> > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > >> > 0x7f3185 wi::int_traits > > > >> >>::decompose(long*, unsigned int, std::pair > > > >> > const&) > > > >> > ../../gcc/gcc/rtl.h:2314 > > > >> > 0x7f3185 wide_int_ref_storage > > >> > false>::wide_int_ref_storage > > > >> >>(std::pair const&) > > > >> > ../../gcc/gcc/wide-int.h:1089 > > > >> > 0x7f3185 generic_wide_int > > > >> >>::generic_wide_int > > > >> >>(std::pair const&) > > > >> > ../../gcc/gcc/wide-int.h:847 > > > >> > 0x7f3185 poly_int<1u, generic_wide_int > > >> > false> > >::poly_int > > > >> >>(poly_int_full, std::pair const&) > > > >> > ../../gcc/gcc/poly-int.h:467 > > > >> > 0x7f3185 poly_int<1u, generic_wide_int > > >> > false> > >::poly_int > > > >> >>(std::pair const&) > > > >> > ../../gcc/gcc/poly-int.h:453 > > > >> > 0x7f3185 wi::to_poly_wide(rtx_def const*, machine_mode) > > > >> > ../../gcc/gcc/rtl.h:2383 > > > >> > 0x7f3185 rtx_vector_builder::step(rtx_def*, rtx_def*) const > > > >> > ../../gcc/gcc/rtx-vector-builder.h:122 > > > >> > 0xfd4e1b vector_builder > > >> > rtx_vector_builder>::elt(unsigned int) const > > > >> > ../../gcc/gcc/vector-builder.h:253 > > > >> > 0xfd4d11 rtx_vector_builder::build() > > > >> > ../../gcc/gcc/rtx-vector-builder.cc:73 > > > >> > 0xc21d9c const_vector_from_tree > > > >> > ../../gcc/gcc/expr.cc:13487 > > > >> > 0xc21d9c expand_expr_real_1(tree_node*, rtx_def*, machine_mode, > > > >> > expand_modifier, rtx_def**, bool) > > > >> > ../../gcc/gcc/expr.cc:11059 > > > >> > 0xaee682 expand_expr(tree_node*, rtx_def*, machine_mode, expand_= modifier) > > > >> > ../../gcc/gcc/expr.h:310 > > > >> > 0xaee682 expand_return > > > >> > ../../gcc/gcc/cfgexpand.cc:3809 > > > >> > 0xaee682 expand_gimple_stmt_1 > > > >> > ../../gcc/gcc/cfgexpand.cc:3918 > > > >> > 0xaee682 expand_gimple_stmt > > > >> > ../../gcc/gcc/cfgexpand.cc:4044 > > > >> > 0xaf28f0 expand_gimple_basic_block > > > >> > ../../gcc/gcc/cfgexpand.cc:6100 > > > >> > 0xaf4996 execute > > > >> > ../../gcc/gcc/cfgexpand.cc:6835 > > > >> > > > > >> > IIUC, the issue is that fold_vec_perm returns a vector having fl= oat element > > > >> > type with res_nelts_per_pattern =3D=3D 3, and later ICE's when i= t tries > > > >> > to derive element v[3], not present in the encoding, while tryin= g to > > > >> > build rtx vector > > > >> > in rtx_vector_builder::build(): > > > >> > for (unsigned int i =3D 0; i < nelts; ++i) > > > >> > RTVEC_ELT (v, i) =3D elt (i); > > > >> > > > > >> > The attached patch tries to fix this by returning false from > > > >> > valid_mask_for_fold_vec_perm_cst if sel has a stepped sequence a= nd > > > >> > input vector has non-integral element type, so for VLA vectors, = it > > > >> > will only build result with dup sequence (nelts_per_pattern < 3)= for > > > >> > non-integral element type. > > > >> > > > > >> > For VLS vectors, this will still work for stepped sequence since= it > > > >> > will then use the "VLS exception" in fold_vec_perm_cst, and set: > > > >> > res_npattern =3D res_nelts and > > > >> > res_nelts_per_pattern =3D 1 > > > >> > > > > >> > and fold the above case to: > > > >> > F foo (F a, F b) > > > >> > { > > > >> > [local count: 1073741824]: > > > >> > return { 0.0, 9.0e+0, 0.0, 0.0 }; > > > >> > } > > > >> > > > > >> > But I am not sure if this is entirely correct, since: > > > >> > tree res =3D out_elts.build (); > > > >> > will canonicalize the encoding and may result in a stepped seque= nce > > > >> > (vector_builder::finalize() may reduce npatterns at the cost of = increasing > > > >> > nelts_per_pattern) ? > > > >> > > > > >> > PS: This issue is now latent after PR111648 fix, since > > > >> > valid_mask_for_fold_vec_perm_cst with sel =3D {1, 0, 1, ...} re= turns > > > >> > false because the corresponding pattern in arg0 is not a natural > > > >> > stepped sequence, and folds correctly using VLS exception. Howev= er, I > > > >> > guess the underlying issue of dealing with non-integral element = types > > > >> > in fold_vec_perm_cst still remains ? > > > >> > > > > >> > The patch passes bootstrap+test with and without SVE on aarch64-= linux-gnu, > > > >> > and on x86_64-linux-gnu. > > > >> > > > >> I think the problem is instead in the way that we're calculating > > > >> res_npatterns and res_nelts_per_pattern. > > > >> > > > >> If the selector is a duplication of { a1, ..., an }, then the > > > >> result will be a duplication of n elements, regardless of the shap= e > > > >> of the other arguments. > > > >> > > > >> Similarly, if the selector is { a1, ...., an } followed by a > > > >> duplication of { b1, ..., bn }, the result be n elements followed > > > >> by a duplication of n elements, regardless of the shape of the oth= er > > > >> arguments. > > > >> > > > >> So for these two cases, res_npatterns and res_nelts_per_pattern > > > >> can come directly from the selector's encoding. > > > >> > > > >> If: > > > >> > > > >> (1) the selector is an n-pattern stepped sequence > > > >> (2) the stepped part of each pattern selects from the same input p= attern > > > >> (3) the stepped part of each pattern does not select the first ele= ment > > > >> of the input pattern, or the full input pattern is stepped > > > >> (your previous patch) > > > >> > > > >> then the result is stepped only if one of the inputs is stepped. > > > >> This is because, if an input pattern has 1 or 2 elements, (3) mean= s > > > >> that each element of the stepped sequence will select the same val= ue, > > > >> as if the selector step had been 0. > > > > Hi Richard, > > > > Thanks for the suggestions! I agree when selector is dup of {a1, ..= . an, ...} or > > > > base elements followed up dup {a1, .. an, b1, ... bn, ...} in that > > > > case we can set > > > > res_nelts_per_pattern from selector's encoding. However even if we = provide > > > > more nelts_per_pattern that necessary, I guess vector_builder::fina= lize() will > > > > canonicalize it to the correct encoding for result ? > > > > > > Right. The elements before finalize is called have to be correct, > > > but they don't need to be canonical or minimal. > > > > > > After the change, we'll build no more elements than we did before. > > > > > > >> So I think the PR could be solved by something like the attached. > > > >> Do you agree? If so, could you base the patch on this instead? > > > >> > > > >> Only tested against the self-tests. > > > >> > > > >> Thanks, > > > >> Richard > > > >> > > > >> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc > > > >> index 40767736389..00fce4945a7 100644 > > > >> --- a/gcc/fold-const.cc > > > >> +++ b/gcc/fold-const.cc > > > >> @@ -10743,27 +10743,37 @@ fold_vec_perm_cst (tree type, tree arg0,= tree arg1, const vec_perm_indices &sel, > > > >> unsigned res_npatterns, res_nelts_per_pattern; > > > >> unsigned HOST_WIDE_INT res_nelts; > > > >> > > > >> - /* (1) If SEL is a suitable mask as determined by > > > >> - valid_mask_for_fold_vec_perm_cst_p, then: > > > >> - res_npatterns =3D max of npatterns between ARG0, ARG1, and S= EL > > > >> - res_nelts_per_pattern =3D max of nelts_per_pattern between > > > >> - ARG0, ARG1 and SEL. > > > >> - (2) If SEL is not a suitable mask, and TYPE is VLS then: > > > >> - res_npatterns =3D nelts in result vector. > > > >> - res_nelts_per_pattern =3D 1. > > > >> - This exception is made so that VLS ARG0, ARG1 and SEL work a= s before. */ > > > >> - if (valid_mask_for_fold_vec_perm_cst_p (arg0, arg1, sel, reason= )) > > > >> - { > > > >> - res_npatterns > > > >> - =3D std::max (VECTOR_CST_NPATTERNS (arg0), > > > >> - std::max (VECTOR_CST_NPATTERNS (arg1), > > > >> - sel.encoding ().npatterns ())); > > > >> + /* First try to implement the fold in a VLA-friendly way. > > > >> + > > > >> + (1) If the selector is simply a duplication of N elements, t= he > > > >> + result is likewise a duplication of N elements. > > > >> + > > > >> + (2) If the selector is N elements followed by a duplication > > > >> + of N elements, the result is too. > > > >> > > > >> - res_nelts_per_pattern > > > >> - =3D std::max (VECTOR_CST_NELTS_PER_PATTERN (arg0), > > > >> - std::max (VECTOR_CST_NELTS_PER_PATTERN (arg1), > > > >> - sel.encoding ().nelts_per_pattern ()= )); > > > >> + (3) If the selector is N elements followed by an interleavin= g > > > >> + of N linear series, the situation is more complex. > > > >> > > > >> + valid_mask_for_fold_vec_perm_cst_p detects whether we > > > >> + can handle this case. If we can, then each of the N line= ar > > > >> + series either (a) selects the same element each time or > > > >> + (b) selects a linear series from one of the input pattern= s. > > > >> + > > > >> + If (b) holds for one of the linear series, the result > > > >> + will contain a linear series, and so the result will have > > > >> + the same shape as the selector. If (a) holds for all of > > > >> + the lienar series, the result will be the same as (2) abo= ve. > > > >> + > > > >> + (b) can only hold if one of the inputs pattern has a > > > >> + stepped encoding. */ > > > >> + if (valid_mask_for_fold_vec_perm_cst_p (arg0, arg1, sel, reason= )) > > > >> + { > > > >> + res_npatterns =3D sel.encoding ().npatterns (); > > > >> + res_nelts_per_pattern =3D sel.encoding ().nelts_per_pattern= (); > > > >> + if (res_nelts_per_pattern =3D=3D 3 > > > >> + && VECTOR_CST_NELTS_PER_PATTERN (arg0) < 3 > > > >> + && VECTOR_CST_NELTS_PER_PATTERN (arg1) < 3) > > > >> + res_nelts_per_pattern =3D 2; > > > > Um, in this case, should we set: > > > > res_nelts_per_pattern =3D max (nelts_per_pattern (arg0), nelts_per_= pattern(arg1)) > > > > if both have nelts_per_pattern =3D=3D 1 ? > > > > > > No, it still needs to be 2 even if arg0 and arg1 are duplicates. > > > E.g. consider a selector that picks the first element of arg0 > > > followed by a duplicate of the first element of arg1. > > > > > > > Also I suppose this matters only for non-integral element type, sin= ce > > > > for integral element type, > > > > vector_cst_elt will return the correct value even if the element is > > > > not explicitly encoded and input vector is dup ? > > > > > > Yeah, but it might help even for integers. If we build fewer > > > elements explicitly, and so read fewer implicitly-encoded inputs, > > > there's less risk of running into: > > > > > > if (!can_div_trunc_p (sel[i], len, &q, &r)) > > > { > > > if (reason) > > > *reason =3D "cannot divide selector element by arg len"; > > > return NULL_TREE; > > > } > > Ah right, thanks for the clarification! > > I am currently away on vacation and will return next Thursday, and > > will post a follow up patch based on your patch. > > Sorry for the delay. > Hi, > Sorry for slow response, I have rebased your patch and added couple of te= sts. > The attached patch resulted in fallout for aarch64/sve/slp_3.c and > aarch64/sve/slp_4.c. > > Specifically for slp_3.c, we didn't fold following case: > arg0, arg1 are dup vectors. > sel =3D { 0, len, 1, len + 1, 2, len + 2, ... } // (npatterns =3D 2, > nelts_per_pattern =3D 3) > because res_nelts_per_pattern was set to 3, and upon encountering 2, > fold_vec_perm_cst returned false. > > With patch, we set res_nelts_per_pattern =3D 2 (since input vectors are > dup), and thus gets folded to: > res =3D { arg0[0], arg1[0], ... } // (2, 1) > > Which results in using ldrqd for loading the result instead of doing > the permutation at runtime with mov and zip1. > I have adjusted the tests for new code-gen. > Does it look OK ? > > There's also this strange failure observed on x86_64, as well as on aarch= 64: > New tests that FAIL (1 tests): > libitm.c++/dropref.C -B > /home/prathamesh.kulkarni/gnu-toolchain/gcc/gnu-964-5/bootstrap-build-aft= er/aarch64-unknown-linux-gnu/./libitm/../libstdc++-v3/src/.libs > execution test > > Looking at dropref.C: > /* { dg-xfail-run-if "unsupported" { *-*-* } } */ > #include > > char *pp; > > int main() > { > __transaction_atomic { > _ITM_dropReferences (pp, 555); > } > return 0; > } > > doesn't seem relevant to VEC_PERM_EXPR folding ? > The patch otherwise passes bootstrap+test on aarch64-linux-gnu with > and without SVE, and on x86_64-linux-gnu. Hi, ping: https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635728.html Thanks, Prathamesh > > Thanks, > Prathamesh > > > > Thanks, > > Prathamesh > > > > > > Thanks, > > > Richard