From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 10E26385841A for ; Wed, 17 Aug 2022 14:42:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 10E26385841A Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-451-yL_vfa2jO-eii3mdWSvlPg-1; Wed, 17 Aug 2022 10:42:25 -0400 X-MC-Unique: yL_vfa2jO-eii3mdWSvlPg-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 684518853C2; Wed, 17 Aug 2022 14:42:01 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.192.41]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 280DA492CA5; Wed, 17 Aug 2022 14:42:00 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 27HEfwgo3133913 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 17 Aug 2022 16:41:58 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 27HEfveX3133912; Wed, 17 Aug 2022 16:41:57 +0200 Date: Wed, 17 Aug 2022 16:41:56 +0200 From: Jakub Jelinek To: Andrew Stubbs Cc: "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH] openmp: fix max_vf setting for amdgcn offloading Message-ID: Reply-To: Jakub Jelinek References: <0e1a740e-46d5-ebfa-36f4-9a069ddf8620@codesourcery.com> MIME-Version: 1.0 In-Reply-To: <0e1a740e-46d5-ebfa-36f4-9a069ddf8620@codesourcery.com> X-Scanned-By: MIMEDefang 2.85 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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 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: Wed, 17 Aug 2022 14:42:29 -0000 On Tue, Jul 12, 2022 at 03:16:35PM +0100, Andrew Stubbs wrote: > --- a/gcc/gimple-loop-versioning.cc > +++ b/gcc/gimple-loop-versioning.cc > @@ -555,7 +555,10 @@ loop_versioning::loop_versioning (function *fn) > unvectorizable code, since it is the largest size that can be > handled efficiently by scalar code. omp_max_vf calculates the > maximum number of bytes in a vector, when such a value is relevant > - to loop optimization. */ > + to loop optimization. > + FIXME: this probably needs to use omp_max_simd_vf when in a target > + region, but how to tell? (And MAX_FIXED_MODE_SIZE is large enough that > + it doesn't actually matter.) */ > m_maximum_scale = estimated_poly_value (omp_max_vf ()); > m_maximum_scale = MAX (m_maximum_scale, MAX_FIXED_MODE_SIZE); I think this shouldn't have the comment added, the use here actually isn't much OpenMP related, it just uses the function because it implements what it wants. > --- a/gcc/omp-general.cc > +++ b/gcc/omp-general.cc > @@ -994,6 +994,24 @@ omp_max_simt_vf (void) > return 0; > } > > +/* Return maximum SIMD width if offloading may target SIMD hardware. */ > + > +int > +omp_max_simd_vf (void) The name is just confusing. omp_max_vf is about the SIMD maximum VF, so if you really want, rename omp_max_vf to omp_max_simd_vf. For the offloading related stuff, IMHO either we put it into that single omp-general.cc function and add a bool argument to it whether it is or might be in offloading region (ordered_maximum from the returned value and the offloading one, but only after the initialy return 1; conditions and adjust callers), or have this separate function, but then IMHO the if (!optimize) return 0; initial test should be if (!optimize || optimize_debug || !flag_tree_loop_optimize || (!flag_tree_loop_vectorize && OPTION_SET_P (flag_tree_loop_vectorize))) return 1; because without that nothing is vectorized, on host nor on offloading targets, and the function should be called omp_max_target_vf or omp_max_target_simd_vf. > +{ > + if (!optimize) > + return 0; > --- a/gcc/omp-low.cc > +++ b/gcc/omp-low.cc > @@ -4646,7 +4646,14 @@ lower_rec_simd_input_clauses (tree new_var, omp_context *ctx, > { > if (known_eq (sctx->max_vf, 0U)) > { > - sctx->max_vf = sctx->is_simt ? omp_max_simt_vf () : omp_max_vf (); > + /* If we are compiling for multiple devices choose the largest VF. */ > + sctx->max_vf = omp_max_vf (); > + if (omp_maybe_offloaded_ctx (ctx)) > + { > + if (sctx->is_simt) > + sctx->max_vf = ordered_max (sctx->max_vf, omp_max_simt_vf ()); > + sctx->max_vf = ordered_max (sctx->max_vf, omp_max_simd_vf ()); > + } This is wrong. If sctx->is_simt, we know it is the SIMT version. So we want to use omp_max_simt_vf (), not maximum of that and something unrelated. Only if !sctx->is_simt, we want to use maximum of omp_max_vf and if omp_maybe_offloaded_ctx also omp_max_target_vf or how it is called (or pass that as argument to omp_max_vf). We have another omp_max_vf () call though, in omp-expand.cc (omp_adjust_chunk_size). That is for schedule (simd: dynamic, 32) and similar, though unlike the omp-low.cc case (where using a larger VF in that case doesn't hurt, it is used for sizing of the maxic arrays that are afterwards resized to the actual size), using too large values in that case is harmful. So dunno if it should take into account offloading vf or not. Maybe if maybe offloading maybe not it should fold to some internal fn call dependent expression that folds to omp_max_vf of the actual target after IPA. Jakub