From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 517F0385801C for ; Tue, 22 Feb 2022 07:58:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 517F0385801C Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 39F4A1F399; Tue, 22 Feb 2022 07:58:15 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 2C870A3B84; Tue, 22 Feb 2022 07:58:15 +0000 (UTC) Date: Tue, 22 Feb 2022 08:58:15 +0100 (CET) From: Richard Biener To: Hongtao Liu cc: GCC Patches , Richard Sandiford , "Liu, Hongtao" Subject: Re: [PATCH 3/3] target/99881 - x86 vector cost of CTOR from integer regs In-Reply-To: Message-ID: References: <20220218140102.13F4E13C91@imap2.suse-dmz.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-10.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: Tue, 22 Feb 2022 07:58:18 -0000 On Tue, 22 Feb 2022, Hongtao Liu wrote: > On Mon, Feb 21, 2022 at 5:10 PM Richard Biener wrote: > > > > On Mon, 21 Feb 2022, Hongtao Liu wrote: > > > > > On Fri, Feb 18, 2022 at 10:01 PM Richard Biener via Gcc-patches > > > wrote: > > > > > > > > This uses the now passed SLP node to the vectorizer costing hook > > > > to adjust vector construction costs for the cost of moving an > > > > integer component from a GPR to a vector register when that's > > > > required for building a vector from components. A cruical difference > > > > here is whether the component is loaded from memory or extracted > > > > from a vector register as in those cases no intermediate GPR is involved. > > > > > > > > The pr99881.c testcase can be Un-XFAILed with this patch, the > > > > pr91446.c testcase now produces scalar code which looks superior > > > > to me so I've adjusted it as well. > > > > > > > > I'm currently re-bootstrapping and testing on x86_64-unknown-linux-gnu > > > > after adding the BIT_FIELD_REF vector extracting special casing. > > > Does the patch handle PR101929? > > > > The patch will regress the testcase posted in PR101929 again: > > > > _255 1 times scalar_store costs 12 in body > > _261 1 times scalar_store costs 12 in body > > _258 1 times scalar_store costs 12 in body > > _264 1 times scalar_store costs 12 in body > > t0_247 + t2_251 1 times scalar_stmt costs 4 in body > > t1_472 + t3_444 1 times scalar_stmt costs 4 in body > > t0_406 - t2_451 1 times scalar_stmt costs 4 in body > > t1_472 - t3_444 1 times scalar_stmt costs 4 in body > > -node 0x4182f48 1 times vec_construct costs 16 in prologue > > -node 0x41882b0 1 times vec_construct costs 16 in prologue > > +node 0x4182f48 1 times vec_construct costs 28 in prologue > > +node 0x41882b0 1 times vec_construct costs 28 in prologue > > t0_406 + t2_451 1 times vector_stmt costs 4 in body > > t1_472 - t3_444 1 times vector_stmt costs 4 in body > > node 0x41829f8 1 times vec_perm costs 4 in body > > _436 1 times vector_store costs 16 in body > > t.c:37:9: note: Cost model analysis for part in loop 0: > > - Vector cost: 60 > > + Vector cost: 84 > > Scalar cost: 64 > > +t.c:37:9: missed: not vectorized: vectorization is not profitable. > > > > We're constructing V4SI from patterns like { _407, _480, _407, _480 } > > where the components are results of integer adds (so the result is > > definitely in a GPR). We are costing the construction as > > 4 * sse_op + 2 * sse_to_integer which with skylake cost is > > 4 * COSTS_N_INSNS (1) + 2 * 6. > > > > Whether the vectorization itself is profitable is likely questionable > > but then it's true that the construction of V4SI is more costly > > in terms of uops than a construction of V4SF. > > > > Now, we can - for the first time - now see the actual construction > > pattern and ideal construction might be two GPR->xmm moves > > + two splats + one unpack or maybe two GPR->xmm moves + one > > unpack + splat of DI (or other means of duplicating the lowpart). > Yes, the patch is technically right. I'm ok with the patch. Thanks, I've pushed it now. I've also tested the suggested adjustment doing diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index b2bf90576d5..acf2cc977b4 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -22595,7 +22595,7 @@ ix86_builtin_vectorization_cost (enum vect_cost_for_stmt type_of_cost, case vec_construct: { /* N element inserts into SSE vectors. */ - int cost = TYPE_VECTOR_SUBPARTS (vectype) * ix86_cost->sse_op; + int cost = (TYPE_VECTOR_SUBPARTS (vectype) - 1) * ix86_cost->sse_op; /* One vinserti128 for combining two SSE vectors for AVX256. */ if (GET_MODE_BITSIZE (mode) == 256) cost += ix86_vec_cost (mode, ix86_cost->addss); successfully (with no effect on the PR101929 case as expected), I will queue that for stage1 since it isn't known to fix any regression (but I will keep it as option in case something pops up). I'll also have a more detailed look into the x264_r case to see if there's something we can do about the regression that will now show up (and I'll watch autotesters). Thanks, Richard. > > It still wouldn't help here of course, and we would not have RTL > > expansion adhere to this scheme. > > > > Trying to capture secondary effects (the FRE opportunity unleashed) > > in costing of this particular SLP subgraph is difficult and probably > > undesirable though. Adjusting any of the target specific costs > > is likely not going to work because of the original narrow profitability > > gap (60 vs. 64). > > > > For the particular kernel the overall vectorization strathegy needs > > to improve (PR98138 is about this I think). I know we've reverted > > the previous change that attempted to cost for GPR -> XMM. It did > > > > case vec_construct: > > { > > /* N element inserts into SSE vectors. */ > > + int cost > > + = TYPE_VECTOR_SUBPARTS (vectype) * (fp ? > > + ix86_cost->sse_op > > + : > > ix86_cost->integer_to_sse); > > + > > - int cost = TYPE_VECTOR_SUBPARTS (vectype) * ix86_cost->sse_op; > > > > thus treated integer_to_sse as GPR insert into vector cost for > > integer components in general while the now proposed change > > _adds_ integer to XMM move costs (but only once for duplicate > > elements and not for the cases we know are from memory / vector regs). > > With the proposed patch we can probably be more "correct" above > > and only cost TYPE_VECTOR_SUBPARTS (vectype) - 1 inserts given > > for FP the first one is free and for int we're costing it separately. > > Again it won't help here. > > > > Thanks, > > Richard. > > > > > > > > > > I suppose we can let autotesters look for SPEC performance fallout. > > > > > > > > OK if testing succeeds? > > > > > > > > Thanks, > > > > Richard. > > > > > > > > 2022-02-18 Richard Biener > > > > > > > > PR tree-optimization/104582 > > > > PR target/99881 > > > > * config/i386/i386.cc (ix86_vector_costs::add_stmt_cost): > > > > Cost GPR to vector register moves for integer vector construction. > > > > > > > > * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c: New. > > > > * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c: Likewise. > > > > * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c: Likewise. > > > > * gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c: Likewise. > > > > * gcc.target/i386/pr99881.c: Un-XFAIL. > > > > * gcc.target/i386/pr91446.c: Adjust to not expect vectorization. > > > > --- > > > > gcc/config/i386/i386.cc | 45 ++++++++++++++++++- > > > > .../costmodel/x86_64/costmodel-pr104582-1.c | 15 +++++++ > > > > .../costmodel/x86_64/costmodel-pr104582-2.c | 13 ++++++ > > > > .../costmodel/x86_64/costmodel-pr104582-3.c | 13 ++++++ > > > > .../costmodel/x86_64/costmodel-pr104582-4.c | 15 +++++++ > > > > gcc/testsuite/gcc.target/i386/pr91446.c | 2 +- > > > > gcc/testsuite/gcc.target/i386/pr99881.c | 2 +- > > > > 7 files changed, 102 insertions(+), 3 deletions(-) > > > > create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c > > > > create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c > > > > create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c > > > > create mode 100644 gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c > > > > > > > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > > > > index 0830dbd7dca..b2bf90576d5 100644 > > > > --- a/gcc/config/i386/i386.cc > > > > +++ b/gcc/config/i386/i386.cc > > > > @@ -22997,7 +22997,7 @@ ix86_vectorize_create_costs (vec_info *vinfo, bool costing_for_scalar) > > > > > > > > unsigned > > > > ix86_vector_costs::add_stmt_cost (int count, vect_cost_for_stmt kind, > > > > - stmt_vec_info stmt_info, slp_tree, > > > > + stmt_vec_info stmt_info, slp_tree node, > > > > tree vectype, int misalign, > > > > vect_cost_model_location where) > > > > { > > > > @@ -23160,6 +23160,49 @@ ix86_vector_costs::add_stmt_cost (int count, vect_cost_for_stmt kind, > > > > stmt_cost = ix86_builtin_vectorization_cost (kind, vectype, misalign); > > > > stmt_cost *= (TYPE_VECTOR_SUBPARTS (vectype) + 1); > > > > } > > > > + else if (kind == vec_construct > > > > + && node > > > > + && SLP_TREE_DEF_TYPE (node) == vect_external_def > > > > + && INTEGRAL_TYPE_P (TREE_TYPE (vectype))) > > > > + { > > > > + stmt_cost = ix86_builtin_vectorization_cost (kind, vectype, misalign); > > > > + unsigned i; > > > > + tree op; > > > > + FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_OPS (node), i, op) > > > > + if (TREE_CODE (op) == SSA_NAME) > > > > + TREE_VISITED (op) = 0; > > > > + FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_OPS (node), i, op) > > > > + { > > > > + if (TREE_CODE (op) != SSA_NAME > > > > + || TREE_VISITED (op)) > > > > + continue; > > > > + TREE_VISITED (op) = 1; > > > > + gimple *def = SSA_NAME_DEF_STMT (op); > > > > + tree tem; > > > > + if (is_gimple_assign (def) > > > > + && CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def)) > > > > + && ((tem = gimple_assign_rhs1 (def)), true) > > > > + && TREE_CODE (tem) == SSA_NAME > > > > + /* A sign-change expands to nothing. */ > > > > + && tree_nop_conversion_p (TREE_TYPE (gimple_assign_lhs (def)), > > > > + TREE_TYPE (tem))) > > > > + def = SSA_NAME_DEF_STMT (tem); > > > > + /* When the component is loaded from memory we can directly > > > > + move it to a vector register, otherwise we have to go > > > > + via a GPR or via vpinsr which involves similar cost. > > > > + Likewise with a BIT_FIELD_REF extracting from a vector > > > > + register we can hope to avoid using a GPR. */ > > > > + if (!is_gimple_assign (def) > > > > + || (!gimple_assign_load_p (def) > > > > + && (gimple_assign_rhs_code (def) != BIT_FIELD_REF > > > > + || !VECTOR_TYPE_P (TREE_TYPE > > > > + (TREE_OPERAND (gimple_assign_rhs1 (def), 0)))))) > > > > + stmt_cost += ix86_cost->sse_to_integer; > > > > + } > > > > + FOR_EACH_VEC_ELT (SLP_TREE_SCALAR_OPS (node), i, op) > > > > + if (TREE_CODE (op) == SSA_NAME) > > > > + TREE_VISITED (op) = 0; > > > > + } > > > > if (stmt_cost == -1) > > > > stmt_cost = ix86_builtin_vectorization_cost (kind, vectype, misalign); > > > > > > > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c > > > > new file mode 100644 > > > > index 00000000000..992a845ad7a > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-1.c > > > > @@ -0,0 +1,15 @@ > > > > +/* { dg-do compile } */ > > > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */ > > > > + > > > > +struct S { unsigned long a, b; } s; > > > > + > > > > +void > > > > +foo (unsigned long *a, unsigned long *b) > > > > +{ > > > > + unsigned long a_ = *a; > > > > + unsigned long b_ = *b; > > > > + s.a = a_; > > > > + s.b = b_; > > > > +} > > > > + > > > > +/* { dg-final { scan-tree-dump "basic block part vectorized" "slp2" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c > > > > new file mode 100644 > > > > index 00000000000..7637cdb4a97 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-2.c > > > > @@ -0,0 +1,13 @@ > > > > +/* { dg-do compile } */ > > > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */ > > > > + > > > > +struct S { unsigned long a, b; } s; > > > > + > > > > +void > > > > +foo (unsigned long a, unsigned long b) > > > > +{ > > > > + s.a = a; > > > > + s.b = b; > > > > +} > > > > + > > > > +/* { dg-final { scan-tree-dump-not "basic block part vectorized" "slp2" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c > > > > new file mode 100644 > > > > index 00000000000..999c4905708 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-3.c > > > > @@ -0,0 +1,13 @@ > > > > +/* { dg-do compile } */ > > > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */ > > > > + > > > > +struct S { double a, b; } s; > > > > + > > > > +void > > > > +foo (double a, double b) > > > > +{ > > > > + s.a = a; > > > > + s.b = b; > > > > +} > > > > + > > > > +/* { dg-final { scan-tree-dump "basic block part vectorized" "slp2" } } */ > > > > diff --git a/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c > > > > new file mode 100644 > > > > index 00000000000..cc471e1ed73 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.dg/vect/costmodel/x86_64/costmodel-pr104582-4.c > > > > @@ -0,0 +1,15 @@ > > > > +/* { dg-do compile } */ > > > > +/* { dg-additional-options "-msse -fdump-tree-slp2-details" } */ > > > > + > > > > +struct S { unsigned long a, b; } s; > > > > + > > > > +void > > > > +foo (signed long *a, unsigned long *b) > > > > +{ > > > > + unsigned long a_ = *a; > > > > + unsigned long b_ = *b; > > > > + s.a = a_; > > > > + s.b = b_; > > > > +} > > > > + > > > > +/* { dg-final { scan-tree-dump "basic block part vectorized" "slp2" } } */ > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr91446.c b/gcc/testsuite/gcc.target/i386/pr91446.c > > > > index 0243ca3ea68..067bf43f698 100644 > > > > --- a/gcc/testsuite/gcc.target/i386/pr91446.c > > > > +++ b/gcc/testsuite/gcc.target/i386/pr91446.c > > > > @@ -21,4 +21,4 @@ foo (unsigned long long width, unsigned long long height, > > > > bar (&t); > > > > } > > > > > > > > -/* { dg-final { scan-assembler-times "vmovdqa\[^\n\r\]*xmm\[0-9\]" 2 } } */ > > > > +/* { dg-final { scan-assembler-times "xmm\[0-9\]" 0 } } */ > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr99881.c b/gcc/testsuite/gcc.target/i386/pr99881.c > > > > index 3e087eb2ed7..a1ec1d1ba8a 100644 > > > > --- a/gcc/testsuite/gcc.target/i386/pr99881.c > > > > +++ b/gcc/testsuite/gcc.target/i386/pr99881.c > > > > @@ -1,7 +1,7 @@ > > > > /* PR target/99881. */ > > > > /* { dg-do compile { target { ! ia32 } } } */ > > > > /* { dg-options "-Ofast -march=skylake" } */ > > > > -/* { dg-final { scan-assembler-not "xmm\[0-9\]" { xfail *-*-* } } } */ > > > > +/* { dg-final { scan-assembler-not "xmm\[0-9\]" } } */ > > > > > > > > void > > > > foo (int* __restrict a, int n, int c) > > > > -- > > > > 2.34.1 > > > > > > > > > > > > > > > > -- > > Richard Biener > > SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, > > Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg) > > > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)