Re: [PATCH v3 1/6] rs6000: Support SSE4.1 "round" intrinsics

[Segher Boessenkool <segher@kernel.crashing.org>]

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