On Wed, Aug 18, 2021 at 02:01:33PM -0500, Bill Schmidt wrote:
> On 8/9/21 3:23 PM, Paul A. Clarke via Gcc-patches wrote:
> Reminder: Please specify what testing you've done and on which
> platforms,
Yes please.
> and where you want to apply the patches (i.e., do you want
> backports?).
Backports are not suitable for features, in general. If nothing is said
I assume no backports (for any patch).
> >+ if (__rounding & _MM_FROUND_NO_EXC)
> >+ {
> >+ /* Save enabled exceptions, and disable all exceptions.
> >+ Pre-POWER9, mffsce decodes to mffs, requiring the additional
> >+ mtfsf, below, to disable exceptions. */
> >+ __asm__ __volatile__ (
> >+ ".machine push; .machine \"power9\"; mffsce %0; .machine pop"
>
>
> As we discussed, this cleverness causes trouble by introducing a
> dependency on a binutils that recognizes .machine "power9". Better to
> just #ifdef this and specify mffsce versus mffs.
It needs testing on all possible pre-p9 systems, because of this
cleverness, too.
( at the end of a line is always wrong btw.
> >+ switch (__rounding)
> >+ {
> >+ case _MM_FROUND_TO_NEAREST_INT:
> >+ __tmp.__fr = __builtin_mffsl ();
>
> Another clever encoding trick, but dangerous. __builtin_mffsl isn't
> guaranteed to be recognized on P8 or earlier, even if it happens to work
> today. Just use mffs and mffsl under #ifdef control.
Yeah. It isn't mentioned in the architecture that this is safe to use,
so if you really have to, it has to be tested everywhere. But better is
to just avoid it.
There are various builtins that automatically create backwards-compatible
code when needed. One of those is __builtin_mffsl. Please use it :-)
> >+ __attribute__((fallthrough));
>
> Well done. :-) A lot of people miss this.
The compiler is supposed to warn whenever you do forget it :-)
> >+ case _MM_FROUND_TO_NEAREST_INT |_MM_FROUND_NO_EXC:
> >+ {
> >+ __builtin_set_fpscr_rn (0b00);
> >+ __r = vec_rint ((__v2df) __A);
> >+ __builtin_set_fpscr_rn (__tmp.__fpscr);
> >+ }
> >+ break;
That layout isn't right. You probably want the break inside the block?
Why do you want a block at all, anyway?
> >+ __asm__ __volatile__ (
> >+ ".machine push; .machine \"power9\"; mffsce %0; .machine pop"
>
> Same issues in this function as above, I won't repeat them all.
You need no quotes either, so you don't need to quote the quotes, fwiw.
Segher
