From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 1A9783858C60 for ; Tue, 12 Oct 2021 22:28:00 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1A9783858C60 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 19CMPntA009799; Tue, 12 Oct 2021 17:26:14 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 19CMPXAl009789; Tue, 12 Oct 2021 17:25:33 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Tue, 12 Oct 2021 17:25:32 -0500 From: Segher Boessenkool To: "Paul A. Clarke" Cc: gcc-patches@gcc.gnu.org, wschmidt@linux.ibm.com Subject: Re: [PATCH v3 1/6] rs6000: Support SSE4.1 "round" intrinsics Message-ID: <20211012222532.GL10333@gate.crashing.org> References: <20211007233906.GQ10333@gate.crashing.org> <20211008010423.GC243632@li-24c3614c-2adc-11b2-a85c-85f334518bdb.ibm.com> <20211008173915.GR10333@gate.crashing.org> <20211008192728.GF243632@li-24c3614c-2adc-11b2-a85c-85f334518bdb.ibm.com> <20211008223111.GU10333@gate.crashing.org> <20211011134617.GG243632@li-24c3614c-2adc-11b2-a85c-85f334518bdb.ibm.com> <20211011162839.GY10333@gate.crashing.org> <20211011173107.GH243632@li-24c3614c-2adc-11b2-a85c-85f334518bdb.ibm.com> <20211011220412.GD10333@gate.crashing.org> <20211012193557.GN243632@li-24c3614c-2adc-11b2-a85c-85f334518bdb.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20211012193557.GN243632@li-24c3614c-2adc-11b2-a85c-85f334518bdb.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=no 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, 12 Oct 2021 22:28:01 -0000 On Tue, Oct 12, 2021 at 02:35:57PM -0500, Paul A. Clarke wrote: > You asked for it. ;-) Boiled down to remove macroisms and code that > should be removed by optimization: Thanks :-) > static __inline __attribute__ ((__always_inline__)) void > libc_feholdsetround_ppc_ctx (struct rm_ctx *ctx, int r) > { > fenv_union_t old; > register fenv_union_t __fr; > __asm__ __volatile__ ("mffscrni %0,%1" : "=f" (__fr.fenv) : "i" (r)); > ctx->env = old.fenv = __fr.fenv; > ctx->updated_status = (r != (old.l & 3)); > } (Should use "n", not "i", only numbers are allowed, not e.g. the address of something. This actually can matter, in unusual cases.) This orders the updating of RN before the store to __fr.fenv . There is no other ordering ensured here. The store to __fr.env obviously has to stay in order with anything that can alias it, if that store isn't optimised away completely later. > static __inline __attribute__ ((__always_inline__)) void > libc_feresetround_ppc (fenv_t *envp) > { > fenv_union_t new = { .fenv = *envp }; > register fenv_union_t __fr; > __fr.l = new.l & 3; > __asm__ __volatile__ ("mffscrn %0,%1" : "=f" (__fr.fenv) : "f" (__fr.fenv)); > } This both reads from and stores to __fr.fenv, the asm has to stay between those two accesses (in the machine code). If the code that actually depends on the modified RN depends onb that __fr.fenv some way, all will be fine. > double > __sin (double x) > { > struct rm_ctx ctx __attribute__ ((cleanup (libc_feresetround_ppc_ctx))); > libc_feholdsetround_ppc_ctx (&ctx, (0)); > /* floating point intensive code. */ > return retval; > } ... but there is no such dependency. The cleanup attribute does not give any such ordering either afaik. > There's not much to it, really. "mffscrni" on the way in to save and set > a required rounding mode, and "mffscrn" on the way out to restore it. Yes. But the code making use of the modified RN needs to have some artificial dependencies with the RN setters, perhaps via __fr.fenv . > > Calling a real function (that does not even need a stack frame, just a > > blr) is not terribly expensive, either. > > Not ideal, better would be better. Yes. But at least it *works* :-) I'll take a stupid, simply, stupidly simple, *robust* solution over some nice, faster,nicely faster way of doing the wrong thing. > > > > > Would creating a __builtin_mffsce be another solution? > > > > > > > > Yes. And not a bad idea in the first place. > > > > > > The previous "Nope" and this "Yes" seem in contradiction. If there is no > > > difference between "asm" and builtin, how does using a builtin solve the > > > problem? > > > > You will have to make the builtin solve it. What a builtin can do is > > virtually unlimited. What an asm can do is not: it just outputs some > > assembler language, and does in/out/clobber constraints. You can do a > > *lot* with that, but it is much more limited than everything you can do > > in the compiler! :-) > > > > The fact remains that there is no way in RTL (or Gimple for that matter) > > to express things like rounding mode changes. You will need to > > artificially make some barriers. > > I know there is __builtin_set_fpscr_rn that generates mffscrn. Or some mtfsb[01]'s, or nasty mffs/mtfsf code, yeah. And it does not provide the ordering either. It *cannot*: you need to cooperate with whatever you are ordering against. There is no way in GCC to say "this is an FP insn and has to stay in order with all FP control writes and FP status reads". Maybe now you see why I like external functions for this :-) > This > is not used in the code above because I believe it first appears in > GCC 9.1 or so, and glibc still supports GCC 6.2 (and it doesn't define > a return value, which would be handy in this case). Does the > implementation of that builtin meet the requirements needed here, > to prevent reordering of FP computation across instantiations of the > builtin? If not, is there a model on which to base an implementation > of __builtin_mffsce (or some preferred name)? It depends on what you are actually ordering, unfortunately. Segher