From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [IPv6:2001:67c:2178:6::1d]) by sourceware.org (Postfix) with ESMTPS id 846893858C5E for ; Wed, 5 Jul 2023 13:29:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 846893858C5E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id A5E8B1FE3E; Wed, 5 Jul 2023 13:29:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1688563764; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ghIBXS0swjrbvPQaQiswEk6JYOlFuVWoPr2R3Id+nWw=; b=0UDBOXIEyS9BvK9VbMqVccjyTfCcoLvcXQG1qwMpCbCp74VCtLrzNRecAwgBJmGTKL0M9v B4ncoSBFIzQccHcswCtDfoYb/8vr4Td2C9yGSeSvoUU1taKMYR7RzzU1L4Y0Ejsexy/IUh iBJKgiGQMoaSnADB8/NPGV815nnChNM= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1688563764; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ghIBXS0swjrbvPQaQiswEk6JYOlFuVWoPr2R3Id+nWw=; b=ucstc6lYvg90dUPt59couozdZ8n5uaK3CVwHn7HfqNZXfh5DDIoLfFlgmuux9riaLMtBYu uM1ToUTfvEMlufCg== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (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 991E02C142; Wed, 5 Jul 2023 13:29:24 +0000 (UTC) Date: Wed, 5 Jul 2023 13:29:24 +0000 (UTC) From: Richard Biener To: Richard Sandiford cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH][RFC] target/110456 - avoid loop masking with zero distance dependences In-Reply-To: Message-ID: References: <5b45a654-529e-4b2e-8a3e-b81ba94d28a1@AM7EUR03FT055.eop-EUR03.prod.protection.outlook.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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, 4 Jul 2023, Richard Sandiford wrote: > Richard Biener writes: > > On Thu, 29 Jun 2023, Richard Biener wrote: > > > >> On Thu, 29 Jun 2023, Richard Sandiford wrote: > >> > >> > Richard Biener writes: > >> > > With applying loop masking to epilogues on x86_64 AVX512 we see > >> > > some significant performance regressions when evaluating SPEC CPU 2017 > >> > > that are caused by store-to-load forwarding fails across outer > >> > > loop iterations when the inner loop does not iterate. Consider > >> > > > >> > > for (j = 0; j < m; ++j) > >> > > for (i = 0; i < n; ++i) > >> > > a[j*n + i] += b[j*n + i]; > >> > > > >> > > with 'n' chosen so that the inner loop vectorized code is fully > >> > > executed by the masked epilogue and that masked epilogue > >> > > storing O > n elements (with elements >= n masked of course). > >> > > Then the masked load performed for the next outer loop iteration > >> > > will get a hit in the store queue but it obviously cannot forward > >> > > so we have to wait for the store to retire. > >> > > > >> > > That causes a significant hit to performance especially if 'n' > >> > > would have made a non-masked epilogue to fully cover 'n' as well > >> > > (say n == 4 for a V4DImode epilogue), avoiding the need for > >> > > store-forwarding and waiting for the retiring of the store. > >> > > > >> > > The following applies a very simple heuristic, disabling > >> > > the use of loop masking when there's a memory reference pair > >> > > with dependence distance zero. That resolves the issue > >> > > (other problematic dependence distances seem to be less common > >> > > at least). > >> > > > >> > > I have applied this heuristic in generic vectorizer code but > >> > > restricted it to non-VL vector sizes. There currently isn't > >> > > a way for the target to request disabling of masking only, > >> > > while we can reject the vectoriztion at costing time that will > >> > > not re-consider the same vector mode but without masking. > >> > > It seems simply re-costing with masking disabled should be > >> > > possible through, we'd just need an indication whether that > >> > > should be done? Maybe always when the current vector mode is > >> > > of fixed size? > >> > > > >> > > I wonder how SVE vectorized code behaves in these situations? > >> > > The affected SPEC CPU 2017 benchmarks were 527.cam4_r and > >> > > 503.bwaves_r though I think both will need a hardware vector > >> > > size covering at least 8 doubles to show the issue. 527.cam4_r > >> > > has 4 elements in the inner loop, 503.bwaves_r 5 IIRC. > >> > > > >> > > Bootstrap / regtest running on x86_64-unknown-linux-gnu. > >> > > > >> > > Any comments? > >> > > > >> > > Thanks, > >> > > Richard. > >> > > > >> > > PR target/110456 > >> > > * tree-vectorizer.h (vec_info_shared::has_zero_dep_dist): New. > >> > > * tree-vectorizer.cc (vec_info_shared::vec_info_shared): > >> > > Initialize has_zero_dep_dist. > >> > > * tree-vect-data-refs.cc (vect_analyze_data_ref_dependence): > >> > > Remember if we've seen a dependence distance of zero. > >> > > * tree-vect-stmts.cc (check_load_store_for_partial_vectors): > >> > > When we've seen a dependence distance of zero and the vector > >> > > type has constant size disable the use of partial vectors. > >> > > --- > >> > > gcc/tree-vect-data-refs.cc | 2 ++ > >> > > gcc/tree-vect-stmts.cc | 10 ++++++++++ > >> > > gcc/tree-vectorizer.cc | 1 + > >> > > gcc/tree-vectorizer.h | 3 +++ > >> > > 4 files changed, 16 insertions(+) > >> > > > >> > > diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc > >> > > index ebe93832b1e..40cde95c16a 100644 > >> > > --- a/gcc/tree-vect-data-refs.cc > >> > > +++ b/gcc/tree-vect-data-refs.cc > >> > > @@ -470,6 +470,8 @@ vect_analyze_data_ref_dependence (struct data_dependence_relation *ddr, > >> > > "dependence distance == 0 between %T and %T\n", > >> > > DR_REF (dra), DR_REF (drb)); > >> > > > >> > > + loop_vinfo->shared->has_zero_dep_dist = true; > >> > > + > >> > > /* When we perform grouped accesses and perform implicit CSE > >> > > by detecting equal accesses and doing disambiguation with > >> > > runtime alias tests like for > >> > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > >> > > index d642d3c257f..3bcbc000323 100644 > >> > > --- a/gcc/tree-vect-stmts.cc > >> > > +++ b/gcc/tree-vect-stmts.cc > >> > > @@ -1839,6 +1839,16 @@ check_load_store_for_partial_vectors (loop_vec_info loop_vinfo, tree vectype, > >> > > using_partial_vectors_p = true; > >> > > } > >> > > > >> > > + if (loop_vinfo->shared->has_zero_dep_dist > >> > > + && TYPE_VECTOR_SUBPARTS (vectype).is_constant ()) > >> > > >> > I don't think it makes sense to treat VLA and VLS differently here. > >> > > >> > But RMW operations are very common, so it seems like we're giving up a > >> > lot on the off chance that the inner loop is applied iteratively > >> > to successive memory locations. > >> > >> Yes ... > >> > >> > Maybe that's still OK for AVX512, where I guess loop masking is more > >> > of a niche use case. But if so, then yeah, I think a hook to disable > >> > masking might be better here. > >> > >> It's a nice use case in that if you'd cost the main vector loop > >> with and without masking the not masked case is always winning. > >> I understand with SVE it would be the same if you fix the > >> vector size but otherwise it would be a win to mask as there's > >> the chance the HW implementation uses bigger vectors than the > >> architected minimum size. > >> > >> So for AVX512 the win is with the epilogue and the case of > >> few scalar iterations where the epilogue iterations play > >> a significant role. Since we only vectorize the epilogue > >> of the main loop but not the epilogue of the epilogue loop > >> we're leaving quite some iterations unvectorized when the > >> main loop uses 512bit vectors and the epilogue 256bit. > >> > >> But that case specifically is also prone to make the cross-iteration > >> issue wrt an outer loop significiant ... > >> > >> I'll see to address this on the costing side somehow. > > > > So with us requiring both partial vector and non-partial vector variants > > working during vectorizable_* I thought it should be possible to > > simply reset LOOP_VINFO_USING_PARTIAL_VECTORS_P and re-cost without > > re-analyzing in case the target deemed the partial vector using loop > > not profitable. > > > > While the following successfully hacks this in-place the question is > > whether the above is really true for VLA vector types? > > Yeah, I think so. We do support full-vector VLA. > > > Besides from this "working", maybe the targets should get to say > > more specifically what they'd like to change - should we make > > the finish_cost () hook have a more elaborate return value or > > provide more hints like we already do with the suggested unroll factor? > > > > I can imagine a "partial vector usage is bad" or "too high VF"? > > But then we probably still want to re-do the final costing part > > so we'd need a way to push away the per-stmt costing we already > > did (sth nicer than the hack below). Maybe make > > 'bool costing_for_scalar' a tri-state, adding 'produce copy > > from current vector cost in vinfo'? > > Not sure I follow the tri-state thing. If the idea is to redo > the vector_costs::add_stmt_costs stuff, could we just export > the cost_vec from vect_analyze_loop_operations and apply the > costs in vect_analyze_loop_costing instead? I think we don't need to re-do the add_stmt_costs, but yes, I guess we could do this and basically initialize the backend cost info only at vect_analyze_loop_costing time. Is that what you suggest? > That seems worthwhile anyway, if we think the costs are going > to depend on partial vs. full vectors. As things stand, we could > already cost with CAN_USE_PARTIAL_VECTORS_P set to 1 and then later > set it to 0. Yeah, at the time we feed stmts to the backed (in vect_analyze_loop_operations) we have not yet committed to using partial vectors or not. What I meant with the tri-state thing is to give the vectorizer an idea how to best recover here without wasting too many cycles. At least costing masking vs. not masking should involve only re-running the costing itself so we could unconditionally try that as the patch does when the masked variant is rejected by the backend (via setting cost to INT_MAX). The question above was whether with VLA vectors we could even code generate with !LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P? > > Anyway - the takeaway is the x86 backend likes a way to disable > > the use of partial vectors for the epilog (or main loop) in some cases. > > An alternative to doing this somehow via the costing hooks would be > > to add a new hook - the specific data dependence check could be > > a hook invoked after dependence checking, initializing > > LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P? > > > > Any good ideas? Anything that comes to your minds that would be > > useful in this area for other targets? > > > > Thanks, > > Richard. > > > > The following applies ontop of the earlier posted patch in this thread. > > > > > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc > > index 8989985700a..a892f72ded3 100644 > > --- a/gcc/config/i386/i386.cc > > +++ b/gcc/config/i386/i386.cc > > @@ -23976,6 +23976,10 @@ ix86_vector_costs::finish_cost (const vector_costs *scalar_costs) > > && (exact_log2 (LOOP_VINFO_VECT_FACTOR (loop_vinfo).to_constant ()) > > > ceil_log2 (LOOP_VINFO_INT_NITERS (loop_vinfo)))) > > m_costs[vect_body] = INT_MAX; > > + > > + if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo) > > + && loop_vinfo->shared->has_zero_dep_dist) > > + m_costs[vect_body] = INT_MAX; > > } > > > > vector_costs::finish_cost (scalar_costs); > > diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc > > index 3b46c58a8d8..85802f07443 100644 > > --- a/gcc/tree-vect-loop.cc > > +++ b/gcc/tree-vect-loop.cc > > @@ -2773,6 +2773,7 @@ start_over: > > } > > > > loop_vinfo->vector_costs = init_cost (loop_vinfo, false); > > + vector_costs saved_costs = *loop_vinfo->vector_costs; > > The target can derive the class and add its own member variables, > so I think we'd need some kind of clone method for this to be safe. > > > /* Analyze the alignment of the data-refs in the loop. > > Fail if a data reference is found that cannot be vectorized. */ > > @@ -3017,6 +3018,8 @@ start_over: > > LOOP_VINFO_USING_SELECT_VL_P (loop_vinfo) = true; > > } > > > > + saved_costs = *loop_vinfo->vector_costs; > > +again_no_partial_vectors: > > /* If we're vectorizing an epilogue loop, the vectorized loop either needs > > to be able to handle fewer than VF scalars, or needs to have a lower VF > > than the main loop. */ > > @@ -3043,6 +3046,17 @@ start_over: > > { > > ok = opt_result::failure_at (vect_location, > > "Loop costings may not be worthwhile.\n"); > > + if (LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo)) > > + { > > + if (dump_enabled_p ()) > > + dump_printf_loc (MSG_NOTE, vect_location, > > + "trying with partial vectors disabled\n"); > > + LOOP_VINFO_CAN_USE_PARTIAL_VECTORS_P (loop_vinfo) = false; > > + LOOP_VINFO_USING_DECREMENTING_IV_P (loop_vinfo) = false; > > + LOOP_VINFO_USING_SELECT_VL_P (loop_vinfo) = false; > > I guess these last two variables should have been set after > vect_determine_partial_vectors_and_peeling rather than before. > Sorry for not noticing earlier. > > The approach seems OK to me otherwise FWIW. OK, will try post-poning the target cost creation and re-feed the cost_vect, that should avoid the need to copy the target cost structure as well. Thanks, Richard. > Thanks, > Richard > > > + *loop_vinfo->vector_costs = saved_costs; > > + goto again_no_partial_vectors; > > + } > > goto again; > > } > > if (!res) > > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc > > index 08d071463fb..003af878ee7 100644 > > --- a/gcc/tree-vect-stmts.cc > > +++ b/gcc/tree-vect-stmts.cc > > @@ -1854,7 +1854,7 @@ check_load_store_for_partial_vectors (loop_vec_info loop_vinfo, tree vectype, > > using_partial_vectors_p = true; > > } > > > > - if (loop_vinfo->shared->has_zero_dep_dist > > + if (0 && loop_vinfo->shared->has_zero_dep_dist > > && TYPE_VECTOR_SUBPARTS (vectype).is_constant ()) > > { > > if (dump_enabled_p ()) > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)