From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x634.google.com (mail-ej1-x634.google.com [IPv6:2a00:1450:4864:20::634]) by sourceware.org (Postfix) with ESMTPS id 64F7C3858D37 for ; Wed, 21 Sep 2022 07:31:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 64F7C3858D37 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-ej1-x634.google.com with SMTP id r18so11541041eja.11 for ; Wed, 21 Sep 2022 00:31:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=weSqWFTr2/JjXR9z4EceB5UXiXs/jxzxPkThz9SRsXg=; b=Z5ZryMXiBsI7Bsa3yylV3Hcn+KHao23MBdfLK1ISzTw5TwkYHV/6bLOPFqrXiOuUq3 XeAAeSRBseyoaCVIvdYODkqv6W18wSvuYV9ne8RhStNv98CNaW8cdroaql7tp3ahs58h /8l+1DYwZm+Udbgfx1Sm03P7DDU3YXZgkru0Yk2MBoIqftN6UHW+Lc5vtDlq9oOQzTID rVi7qIFTrA8ipC6+lci3GQNPMmbG7EDgXmMp2SRrFboyGMAvkyOhcA716YIiXLwYfHON N8OWgZTeL7UXhKudPUilB/naZI4ihvOkIx/5qc+8WZuwuyB8bXcFx8wUUph6+21EYNRr w1yg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=weSqWFTr2/JjXR9z4EceB5UXiXs/jxzxPkThz9SRsXg=; b=O1ve7eZXADFD+qMDn9mLQ6wHyY/S+RTMpCWPjCco5tZwNKqiznRLX3qk8rkP87X+4z ORIquvZrPgjKVBQpYSXr1zEw/QqUAbK/HDrB0PRjSSsUrg6zFOxGUD2pkwKM8h+9nozX JlO1Sf7dzAjw7xnQbensHzcqcRDBwhfJZ26KlcrNSAUQXdJW8umm/MXgrdz65+Y+jG/i B21W7tp/z/0a7jK6xRCDgN185sk07ZJC7vp0592bbOSZ3lML3lo6Aan8DS4jM1d3PA98 zbL6THt3Mozqvu5ctk4BYXmleAKkTtVFOGdI4z0L5+RVAZx8LPn7BfPjl7WcftjJG2Ll VOLw== X-Gm-Message-State: ACrzQf2wZ1aAuwTxXLy94WLCY43pdncVExpMvLFdpSriNGhhl3ikaLdr Ovp40bGgI7max+OQvcmwz3imT6OC7pDPiEz4YAw= X-Google-Smtp-Source: AMsMyM4IMuhQlEKsWjVYRhSnllbpXoW8DacKiU4sHgPgmAUzwzv1W2WW6YrIGMdeeXPLJtVwG/fa/v3KRc/qxoHMv50= X-Received: by 2002:a17:907:971e:b0:77b:b0f3:6473 with SMTP id jg30-20020a170907971e00b0077bb0f36473mr19492654ejc.754.1663745502837; Wed, 21 Sep 2022 00:31:42 -0700 (PDT) MIME-Version: 1.0 References: <20220920022133.64778-1-hongtao.liu@intel.com> In-Reply-To: <20220920022133.64778-1-hongtao.liu@intel.com> From: Richard Biener Date: Wed, 21 Sep 2022 09:31:29 +0200 Message-ID: Subject: Re: [PATCH] Fix incorrect handle in vectorizable_induction for mixed induction type. To: liuhongt Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.1 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 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 Tue, Sep 20, 2022 at 4:24 AM liuhongt via Gcc-patches wrote: > > The codes in vectorizable_induction for slp_node assume all phi_info > have same induction type(vect_step_op_add), but since we support > nonlinear induction, it could be wrong handled. > So the patch return false when slp_node has mixed induction type. > > Note codes in other place will still vectorize the induction with > separate iv update and vec_perm. But slp_node handle in > vectorizable_induction will be more optimal when all induction type > are the same, it will update ivs with one operation instead of > separate iv updates and permutation. > > Bootstrapped and regtested on x86_64-pc-linux-gnu{-m32,}. > Ok for trunk? > > gcc/ChangeLog: > > PR tree-optimization/103144 > * tree-vect-loop.cc (vectorizable_induction): Return false for > slp_node with mixed induction type. > > gcc/testsuite/ChangeLog: > > * gcc.target/i386/pr103144-mix-1.c: New test. > * gcc.target/i386/pr103144-mix-2.c: New test. > --- > .../gcc.target/i386/pr103144-mix-1.c | 17 +++++++++ > .../gcc.target/i386/pr103144-mix-2.c | 35 +++++++++++++++++++ > gcc/tree-vect-loop.cc | 34 ++++++++++++++---- > 3 files changed, 79 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/i386/pr103144-mix-1.c > create mode 100644 gcc/testsuite/gcc.target/i386/pr103144-mix-2.c > > diff --git a/gcc/testsuite/gcc.target/i386/pr103144-mix-1.c b/gcc/testsuite/gcc.target/i386/pr103144-mix-1.c > new file mode 100644 > index 00000000000..b292d66ef71 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/pr103144-mix-1.c > @@ -0,0 +1,17 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O3 -fdump-tree-optimized" } */ > +/* { dg-final { scan-tree-dump-times "VEC_PERM_EXPR" 2 "optimized" } } */ > +/* For induction variable with differernt induction type(vect_step_op_add, vect_step_op_neg), > + It should't be handled in vectorizable_induction with just 1 single iv update(addition.), > + separate iv update and vec_perm are needed. */ > +int > +__attribute__((noipa)) > +foo (int* p, int c, int n) > +{ > + for (int i = 0; i != n; i++) > + { > + p[2* i]= i; > + p[2 * i+1] = c; > + c = -c; > + } > +} > diff --git a/gcc/testsuite/gcc.target/i386/pr103144-mix-2.c b/gcc/testsuite/gcc.target/i386/pr103144-mix-2.c > new file mode 100644 > index 00000000000..b7043d59aec > --- /dev/null > +++ b/gcc/testsuite/gcc.target/i386/pr103144-mix-2.c > @@ -0,0 +1,35 @@ > +/* { dg-do run } */ > +/* { dg-options "-O2 -mavx2 -ftree-vectorize -fvect-cost-model=unlimited -mprefer-vector-width=256" } */ > +/* { dg-require-effective-target avx2 } */ > + > +#include "avx2-check.h" > +#include > +#include "pr103144-mix-1.c" > + > +typedef int v8si __attribute__((vector_size(32))); > + > +#define N 34 > +void > +avx2_test (void) > +{ > + int* epi32_exp = (int*) malloc (N * sizeof (int)); > + int* epi32_dst = (int*) malloc (N * sizeof (int)); > + > + __builtin_memset (epi32_exp, 0, N * sizeof (int)); > + int b = 8; > + v8si init1 = __extension__(v8si) { 0, b, 1, -b, 2, b, 3, -b }; > + v8si init2 = __extension__(v8si) { 4, b, 5, -b, 6, b, 7, -b }; > + v8si init3 = __extension__(v8si) { 8, b, 9, -b, 10, b, 11, -b }; > + v8si init4 = __extension__(v8si) { 12, b, 13, -b, 14, b, 15, -b }; > + memcpy (epi32_exp, &init1, 32); > + memcpy (epi32_exp + 8, &init2, 32); > + memcpy (epi32_exp + 16, &init3, 32); > + memcpy (epi32_exp + 24, &init4, 32); > + epi32_exp[32] = 16; > + epi32_exp[33] = b; > + foo (epi32_dst, b, N / 2); > + if (__builtin_memcmp (epi32_dst, epi32_exp, N * sizeof (int)) != 0) > + __builtin_abort (); > + > + return; > +} > diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc > index 9c434b66c5b..c7050a47c1c 100644 > --- a/gcc/tree-vect-loop.cc > +++ b/gcc/tree-vect-loop.cc > @@ -9007,14 +9007,34 @@ vectorizable_induction (loop_vec_info loop_vinfo, > iv_loop = loop; > gcc_assert (iv_loop == (gimple_bb (phi))->loop_father); > > - if (slp_node && !nunits.is_constant ()) > + if (slp_node) > { > - /* The current SLP code creates the step value element-by-element. */ > - if (dump_enabled_p ()) > - dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > - "SLP induction not supported for variable-length" > - " vectors.\n"); > - return false; > + if (!nunits.is_constant ()) > + { > + /* The current SLP code creates the step value element-by-element. */ > + if (dump_enabled_p ()) > + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > + "SLP induction not supported for variable-length" > + " vectors.\n"); > + return false; > + } > + > + stmt_vec_info phi_info; > + FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_STMTS (slp_node), i, phi_info) > + { > + if (STMT_VINFO_LOOP_PHI_EVOLUTION_TYPE (phi_info) != vect_step_op_add) While it works to check this here the canonical place to match up def types is in vect_get_and_check_slp_defs. There we do /* Not first stmt of the group, check that the def-stmt/s match the def-stmt/s of the first stmt. Allow different definition types for reduction chains: the first stmt must be a vect_reduction_def (a phi node), and the rest end in the reduction chain. */ if ((!vect_def_types_match (oprnd_info->first_dt, dt) && !(oprnd_info->first_dt == vect_reduction_def && !STMT_VINFO_DATA_REF (stmt_info) the induction def type match now needs to also compare STMT_VINFO_LOOP_PHI_EVOLUTION_TYPE it seems. Note we also fail to do this for STMT_VINFO_REDUC_TYPE (but that's eventually only computed too late). Being able to reject this during SLP discovery means that if you'd have x = i + j; y = j + i; with two different induction types for 'i' and 'j', SLP discovery can try commutative swapping to match the inductions up for x and y and successfully SLP the operation. That said, I can see the checking condition becoming more ugly (please don't try to amend vect_def_types_match itself), adding something like || (oprnd_info->first_dt == vect_induction_def && STMT_VINFO_LOOP_PHI_EVOLUTION_TYPE (...) != ... (stmts[0])) Can you try doing that? Thanks, Richard. > + { > + /* The below SLP code assume all induction type to be the same. > + But slp in other place will still vectorize the loop via updating > + iv update separately + vec_perm, but not from below codes. */ > + if (dump_enabled_p ()) > + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, > + "SLP induction not supported for mixed induction type" > + " vectors.\n"); > + return false; > + } > + } > + > } > > if (FLOAT_TYPE_P (vectype) && !param_vect_induction_float) > -- > 2.18.1 >