Re: [RFC PATCH] target/ppc: do not silence snan in xscvspdpn

[Matheus K. Ferst]

On 15/12/2021 12:55, Alex Bennée wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>
> > On 12/13/21 21:15, Matheus K. Ferst wrote:
> > > On 13/12/2021 09:36, Philippe Mathieu-Daudé wrote:
> > > > On 12/13/21 13:13, matheus.ferst@eldorado.org.br wrote:
> > > > > From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > > > >
> > > > > The non-signalling versions of VSX scalar convert to shorter/longer
> > > > > precision insns doesn't silence SNaNs in the hardware. We are currently
> > > > > honoring this behavior in xscvdpspn, since helper_xscvdpspn handles the
> > > > > conversion with extracts/deposits/etc. OTOH, xscvspdpn uses
> > > > > float32_to_float64 that calls parts_float_to_float, which uses
> > > > > parts_return_nan that finally calls parts_silence_nan if the input is an
> > > > > SNaN.
> > > > >
> > > > > To address this problem, this patch adds a new float_status flag
> > > > > (return_snan) to avoid the call to parts_silence_nan in the
> > > > > float_class_snan case of parts_return_nan.
> > > > >
> > > > > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > > > > ---
> > > > > This behavior was observed in a Power9 and can be reproduced with the
> > > > > following code:
> > > > >
> > > > > int main(void)
> > > > > {
> > > > >       __uint128_t t, b = 0x7f80000200000000;
> > > > >
> > > > >       asm("xscvspdpn %x0, %x1\n\t"
> > > > >           : "=wa" (t)
> > > > >           : "wa" (b << 64));
> > > > >       printf("0x%016" PRIx64 "%016" PRIx64 "\n",
> > > > >              (uint64_t)(t >> 64), (uint64_t)t);
> > > > >
> > > > >       b = 0x7ff0000000000002;
> > > > >       asm("xscvdpspn %x0, %x1\n\t"
> > > > >           : "=wa" (t)
> > > > >           : "wa" (b << 64));
> > > > >       printf("0x%016" PRIx64 "%016" PRIx64 "\n",
> > > > >              (uint64_t)(t >> 64), (uint64_t)t);
> > > > >
> > > > >       return 0;
> > > > > }
> > > >
> > > > Why not add this test in tests/tcg/ppc64le/ ?
> > >
> > > I'll add it in v2. Is it ok to use __uint128_t in tests?
> >
> > What about:
> >
> >    int main(void)
> >    {
> >    #ifndef __SIZEOF_INT128__
> >        printf("uint128_t not available, skipping...\n");
> >    #else
> >        ...
> >    #endif
> >        return 0;
> >    }
>
> We have a tests/tcg/configure.sh which does feature tests although it is
> mainly testing for the presence of compiler target flags.  We do this
> because while the docker compilers are all pretty modern the user might
> be using their own cross compiler.
>
> I'm generally not keen on the tests silently skipping because it gives a
> false impression things have been tested. If it is possible to avoid
> INT128 shenanigans to load the values into the inline assembler lets do
> that, otherwise lets feature test and ifdef a skip-test in the makefile.
>
> --
> Alex Bennée

I think we can pass the value via register and use mtvsrd/mfvsrd to 
avoid the INT128 part.

There was a v2 of this patch, but I messed up the CC and only included 
get_maintainer.pl result and Philippe. The patch is now applied, so I'll 
send this change as a follow-up.

Thanks,
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>