From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12b.google.com (mail-lf1-x12b.google.com [IPv6:2a00:1450:4864:20::12b]) by sourceware.org (Postfix) with ESMTPS id 5149F3858023 for ; Wed, 26 Jul 2023 10:02:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5149F3858023 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lf1-x12b.google.com with SMTP id 2adb3069b0e04-4fe0fe622c3so840539e87.2 for ; Wed, 26 Jul 2023 03:02:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1690365769; x=1690970569; 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=ansm1RX71zf3z17qW+KsuO8SbBys5GR68sKn+5zmXqk=; b=SU+1xN7Q3G5EuqG2yabXrjrohqFhooQqHZ801shZb0jnnahWLqP7OylK0BisxiEmtK GLmEhmrtuyPgTwLUldI+/Kz2g2EXcmslvTzExr5NSg00jUSY47NboQEY9+MRJa4lzMb4 BD8hVeZ+82iFfi04Eani0NkpZ9yLhgbSJaLYpyzB+nW5dPbrju3AugH8BLMzofUttmgE lp/YtfK4LABppMapI783JEqeRKoarR5O6K86eCvQNPo8mnrrT3xUW9Lj9zfH6wdCf7TQ rFcL1S4QD8ZCxDwJ9cfxBcFruY4EhJ2W4bzfJvkeMN5CS4voSWVFlv6mWjD0vfSEM1hv dwaA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1690365769; x=1690970569; 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=ansm1RX71zf3z17qW+KsuO8SbBys5GR68sKn+5zmXqk=; b=aPPd4blhiQz1f0YI/TfrFCWCavf2ccoKLeoI1CfzTmTuPuM3sO9XZgW3XYotqoaBtk XW2R63ZHju2jG0QLDMiRLHbJKvPzyEdJazaGY3BQQEoc8a8/hcKMy7TMfweY1pvMsxrw xo7OF0qPdsbMlh3iGRXM8hspnTYmedNKFfCcawBbJ9kkjKR5IJq3j/48gKgt3Nilfqtf Xep3Az3EtehBEMlHsJTsNJTc3uCQGE7KoDttT+hWUp9ITuXiVVhM1o8WWgpFvpDpNdOr 7kA6zK1tDfwsmGwcHp4cQErQ/Dyn4eJtfH6Eoh6FagkL42jShXbqqxSRmY8XoLs7eQYv 0iyw== X-Gm-Message-State: ABy/qLabyEA3KbRhURnf40xCBvPYvRFTYDhZTNI2uZeGDlCorC8EbDUe k40rOmxiuq/XdcYggUKROC6tsTYwp+Mj70smbwk= X-Google-Smtp-Source: APBJJlGcn2+2qAXiaQ4tBNTTvJUSQ9GRD7v/t/J/NGqugOfEClz8dSgRAg70C+7TRoID58XxjoDpzXMeDOScwjHMVjA= X-Received: by 2002:a2e:920d:0:b0:2b5:7fba:18ac with SMTP id k13-20020a2e920d000000b002b57fba18acmr941640ljg.48.1690365768633; Wed, 26 Jul 2023 03:02:48 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Wed, 26 Jul 2023 12:02:01 +0200 Message-ID: Subject: Re: [PATCH] AArch64: Do not increase the vect reduction latency by multiplying count [PR110625] To: Richard Biener , Hao Liu OS , "GCC-patches@gcc.gnu.org" , richard.sandiford@arm.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.5 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE,URIBL_BLACK 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, Jul 26, 2023 at 11:14=E2=80=AFAM Richard Sandiford wrote: > > Richard Biener writes: > > On Wed, Jul 26, 2023 at 4:02=E2=80=AFAM Hao Liu OS via Gcc-patches > > wrote: > >> > >> > When was STMT_VINFO_REDUC_DEF empty? I just want to make sure that = we're not papering over an issue elsewhere. > >> > >> Yes, I also wonder if this is an issue in vectorizable_reduction. Bel= ow is the the gimple of "gcc.target/aarch64/sve/cost_model_13.c": > >> > >> : > >> # res_18 =3D PHI > >> # i_20 =3D PHI > >> _1 =3D (long unsigned int) i_20; > >> _2 =3D _1 * 2; > >> _3 =3D x_14(D) + _2; > >> _4 =3D *_3; > >> _5 =3D (unsigned short) _4; > >> res.0_6 =3D (unsigned short) res_18; > >> _7 =3D _5 + res.0_6; <-- The current stm= t_info > >> res_15 =3D (short int) _7; > >> i_16 =3D i_20 + 1; > >> if (n_11(D) > i_16) > >> goto ; > >> else > >> goto ; > >> > >> : > >> goto ; > >> > >> It looks like that STMT_VINFO_REDUC_DEF should be "res_18 =3D PHI "? > >> The status here is: > >> STMT_VINFO_REDUC_IDX (stmt_info): 1 > >> STMT_VINFO_REDUC_TYPE (stmt_info): TREE_CODE_REDUCTION > >> STMT_VINFO_REDUC_VECTYPE (stmt_info): 0x0 > > > > Not all stmts in the SSA cycle forming the reduction have > > STMT_VINFO_REDUC_DEF set, > > only the last (latch def) and live stmts have at the moment. > > Ah, thanks. In that case, Hao, I think we can avoid the ICE by changing: > > if ((kind =3D=3D scalar_stmt || kind =3D=3D vector_stmt || kind =3D=3D = vec_to_scalar) > && vect_is_reduction (stmt_info)) > > to: > > if ((kind =3D=3D scalar_stmt || kind =3D=3D vector_stmt || kind =3D=3D = vec_to_scalar) > && STMT_VINFO_LIVE_P (stmt_info) > && vect_is_reduction (stmt_info)) > > instead of using a null check. But as seen you will miss stmts that are part of the reduction then? In principle we could put STMT_VINFO_REDUC_DEF to other stmts as well. See vectorizable_reduction in the while (reduc_def !=3D PHI_RESULT (reduc_def_phi)) loop. > I see that vectorizable_reduction calculates a reduc_chain_length. > Would it be OK to store that in the stmt_vec_info? I suppose the > AArch64 code should be multiplying by that as well. (It would be a > separate patch from this one though.) I don't think that's too relevant here (it also counts noop conversions). Richard. > > Richard > > > > > > Richard. > > > >> Thanks, > >> Hao > >> > >> ________________________________________ > >> From: Richard Sandiford > >> Sent: Tuesday, July 25, 2023 17:44 > >> To: Hao Liu OS > >> Cc: GCC-patches@gcc.gnu.org > >> Subject: Re: [PATCH] AArch64: Do not increase the vect reduction laten= cy by multiplying count [PR110625] > >> > >> Hao Liu OS writes: > >> > Hi, > >> > > >> > Thanks for the suggestion. I tested it and found a gcc_assert failu= re: > >> > gcc.target/aarch64/sve/cost_model_13.c (internal compiler error:= in info_for_reduction, at tree-vect-loop.cc:5473) > >> > > >> > It is caused by empty STMT_VINFO_REDUC_DEF. > >> > >> When was STMT_VINFO_REDUC_DEF empty? I just want to make sure that > >> we're not papering over an issue elsewhere. > >> > >> Thanks, > >> Richard > >> > >> So, I added an extra check before checking single_defuse_cycle. The = updated patch is below. Is it OK for trunk? > >> > > >> > --- > >> > > >> > The new costs should only count reduction latency by multiplying cou= nt for > >> > single_defuse_cycle. For other situations, this will increase the r= eduction > >> > latency a lot and miss vectorization opportunities. > >> > > >> > Tested on aarch64-linux-gnu. > >> > > >> > gcc/ChangeLog: > >> > > >> > PR target/110625 > >> > * config/aarch64/aarch64.cc (count_ops): Only '* count' for > >> > single_defuse_cycle while counting reduction_latency. > >> > > >> > gcc/testsuite/ChangeLog: > >> > > >> > * gcc.target/aarch64/pr110625_1.c: New testcase. > >> > * gcc.target/aarch64/pr110625_2.c: New testcase. > >> > --- > >> > gcc/config/aarch64/aarch64.cc | 13 ++++-- > >> > gcc/testsuite/gcc.target/aarch64/pr110625_1.c | 46 ++++++++++++++++= +++ > >> > gcc/testsuite/gcc.target/aarch64/pr110625_2.c | 14 ++++++ > >> > 3 files changed, 69 insertions(+), 4 deletions(-) > >> > create mode 100644 gcc/testsuite/gcc.target/aarch64/pr110625_1.c > >> > create mode 100644 gcc/testsuite/gcc.target/aarch64/pr110625_2.c > >> > > >> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarc= h64.cc > >> > index 560e5431636..478a4e00110 100644 > >> > --- a/gcc/config/aarch64/aarch64.cc > >> > +++ b/gcc/config/aarch64/aarch64.cc > >> > @@ -16788,10 +16788,15 @@ aarch64_vector_costs::count_ops (unsigned = int count, vect_cost_for_stmt kind, > >> > { > >> > unsigned int base > >> > =3D aarch64_in_loop_reduction_latency (m_vinfo, stmt_info, m_v= ec_flags); > >> > - > >> > - /* ??? Ideally we'd do COUNT reductions in parallel, but unfo= rtunately > >> > - that's not yet the case. */ > >> > - ops->reduction_latency =3D MAX (ops->reduction_latency, base = * count); > >> > + if (STMT_VINFO_REDUC_DEF (stmt_info) > >> > + && STMT_VINFO_FORCE_SINGLE_CYCLE ( > >> > + info_for_reduction (m_vinfo, stmt_info))) > >> > + /* ??? Ideally we'd use a tree to reduce the copies down to 1 = vector, > >> > + and then accumulate that, but at the moment the loop-carrie= d > >> > + dependency includes all copies. */ > >> > + ops->reduction_latency =3D MAX (ops->reduction_latency, base *= count); > >> > + else > >> > + ops->reduction_latency =3D MAX (ops->reduction_latency, base); > >> > } > >> > > >> > /* Assume that multiply-adds will become a single operation. */ > >> > diff --git a/gcc/testsuite/gcc.target/aarch64/pr110625_1.c b/gcc/tes= tsuite/gcc.target/aarch64/pr110625_1.c > >> > new file mode 100644 > >> > index 00000000000..0965cac33a0 > >> > --- /dev/null > >> > +++ b/gcc/testsuite/gcc.target/aarch64/pr110625_1.c > >> > @@ -0,0 +1,46 @@ > >> > +/* { dg-do compile } */ > >> > +/* { dg-options "-Ofast -mcpu=3Dneoverse-n2 -fdump-tree-vect-detail= s -fno-tree-slp-vectorize" } */ > >> > +/* { dg-final { scan-tree-dump-not "reduction latency =3D 8" "vect"= } } */ > >> > + > >> > +/* Do not increase the vector body cost due to the incorrect reduct= ion latency > >> > + Original vector body cost =3D 51 > >> > + Scalar issue estimate: > >> > + ... > >> > + reduction latency =3D 2 > >> > + estimated min cycles per iteration =3D 2.000000 > >> > + estimated cycles per vector iteration (for VF 2) =3D 4.000000 > >> > + Vector issue estimate: > >> > + ... > >> > + reduction latency =3D 8 <-- Too large > >> > + estimated min cycles per iteration =3D 8.000000 > >> > + Increasing body cost to 102 because scalar code would issue mor= e quickly > >> > + ... > >> > + missed: cost model: the vector iteration cost =3D 102 divided = by the scalar iteration cost =3D 44 is greater or equal to the vectorizatio= n factor =3D 2. > >> > + missed: not vectorized: vectorization not profitable. */ > >> > + > >> > +typedef struct > >> > +{ > >> > + unsigned short m1, m2, m3, m4; > >> > +} the_struct_t; > >> > +typedef struct > >> > +{ > >> > + double m1, m2, m3, m4, m5; > >> > +} the_struct2_t; > >> > + > >> > +double > >> > +bar (the_struct2_t *); > >> > + > >> > +double > >> > +foo (double *k, unsigned int n, the_struct_t *the_struct) > >> > +{ > >> > + unsigned int u; > >> > + the_struct2_t result; > >> > + for (u =3D 0; u < n; u++, k--) > >> > + { > >> > + result.m1 +=3D (*k) * the_struct[u].m1; > >> > + result.m2 +=3D (*k) * the_struct[u].m2; > >> > + result.m3 +=3D (*k) * the_struct[u].m3; > >> > + result.m4 +=3D (*k) * the_struct[u].m4; > >> > + } > >> > + return bar (&result); > >> > +} > >> > diff --git a/gcc/testsuite/gcc.target/aarch64/pr110625_2.c b/gcc/tes= tsuite/gcc.target/aarch64/pr110625_2.c > >> > new file mode 100644 > >> > index 00000000000..7a84aa8355e > >> > --- /dev/null > >> > +++ b/gcc/testsuite/gcc.target/aarch64/pr110625_2.c > >> > @@ -0,0 +1,14 @@ > >> > +/* { dg-do compile } */ > >> > +/* { dg-options "-Ofast -mcpu=3Dneoverse-n2 -fdump-tree-vect-detail= s -fno-tree-slp-vectorize" } */ > >> > +/* { dg-final { scan-tree-dump "reduction latency =3D 8" "vect" } }= */ > >> > + > >> > +/* The reduction latency should be multiplied by the count for > >> > + single_defuse_cycle. */ > >> > + > >> > +long > >> > +f (long res, short *ptr1, short *ptr2, int n) > >> > +{ > >> > + for (int i =3D 0; i < n; ++i) > >> > + res +=3D (long) ptr1[i] << ptr2[i]; > >> > + return res; > >> > +}