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

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

On Fri, Oct 08, 2021 at 02:27:28PM -0500, Paul A. Clarke wrote:
> On Fri, Oct 08, 2021 at 12:39:15PM -0500, Segher Boessenkool wrote:
> > This is not a basic asm (it contains a ":"; that is not just an easy way
> > to see it, it is the *definition* of basic vs. extended asm).
> 
> Ah, basic vs extended. I learned something today... thanks for your
> patience!

To expand a little: any asm with operands is extended asm.  And without
operands can be either:  asm("eieio");  is basic, while  asm("eieio" : );
is extended.  This matters because semantics are a bit different.

> I see. Thanks for the reference. If I understand correctly, volatile
> prevents some optimizations based on the defined inputs/outputs, but
> the asm could still be subject to reordering.

"asm volatile" means there is a side effect in the asm.  This means that
it has to be executed on the real machine the same as on the abstract
machine, with the side effects in the same order.

It can still be reordered, modulo those restrictions.  It can be merged
with an identical asm as well.  And the compiler can split this into two
identical asms on two paths.

In this case you might want a side effect (the instructions writes to
the FPSCR after all).  But you need this to be tied to the FP code that
you want the flags to be changed for, and to the restore of the flags,
and finally you need to prevent other FP code from being scheduled in
between.

You need more for that than just volatile, and the solution may well
make volatile not wanted: tying the insns together somehow will
naturally make the flags restored to a sane situation again, so the
whole group can be removed if you want, etc.

> In this particular case, I don't think it's an issue with respect to
> reordering.  The code in question is:
> +      __asm__ __volatile__ ("mffsce %0" : "=f" (__fpscr_save.__fr));
> +      __enables_save.__fpscr = __fpscr_save.__fpscr & 0xf8;
> 
> The output (__fpscr_save) is a source for the following assignment,
> so the order should be respected, no?

Other FP code can be interleaved, and then do the wrong thing.

> With respect to volatile, I worry about removing it, because I do
> indeed need that instruction to execute in order to clear the FPSCR
> exception enable bits. That side-effect is not otherwise known to the
> compiler.

Yes.  But as said above, volatile isn't enough to get this to behave
correctly.

The easiest way out is to write this all in one piece of (inline) asm.

> > > > You do not need any of that __ either.
> > > 
> > > I'm surprised that I don't. A .h file needs to be concerned about the
> > > namespace it inherits, no?
> > 
> > These are local variables in a function though.  You get such
> > complexities in macros, but never in functions, where everything is
> > scoped.  Local variables are a great thing.  And macros are a bad thing!
> 
> They are local variables in a function *in an include file*, though.
> If a user's preprocessor macro just happens to match a local variable name
> there could be problems, right?

Of course.  This is why traditionally macro names are ALL_CAPS :-)  So
in practice it doesn't matter, and in practice many users use __ names
themselves as well.

But you are right.  I just don't see it will help practically :-(


Segher