Re: [RFC PATCH 0/3] tests/tcg/ppc64le: fix the build of TCG tests with Clang

[Matheus K. Ferst]

On 09/02/2022 09:31, Cédric Le Goater wrote:
> Hello Matheus,
>
> [ Adding Miroslav ]
>
> On 2/8/22 21:31, matheus.ferst@eldorado.org.br wrote:
> > From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> >
> > Based-on: 
> > https://lists.gnu.org/archive/html/qemu-devel/2022-01/msg06506.html
> >
> > As the configuration scripts used -mbig and -mlittle, building PPC tests
> > with Clang was silently skipped. With the patch to fix these options[1],
> > "make check-tcg" fails because of build and runtime errors. This patch
> > series tries to fix some of these problems.
> >
> > The first patch fixes "tests/tcg/ppc64le/mtfsf.c" by removing the
> > GCC-only builtins used to emit mtfsf and mffs. We can emit these insns
> > with inline asm instead.
> >
> > The second patch addresses differences in the output of float_madds.c.
> > The __builtin_fmaf used in this test emits fmadds with GCC and xsmaddasp
> > with LLVM. The first insn had rounding errors fixed in
> > d04ca895dc7f ("target/ppc: Add helpers for fmadds et al"), we apply
> > a similar fix to xsmaddasp.
> >
> > Then we have the build errors of tests/tcg/ppc64le/bcdsub.c. According
> > to GCC docs[2], the '-mpower8-vector' flag provides some bcdsub
> > builtins, so it'd be reasonable to assume that the rest of the toolchain
> > knows about the insn if the compiler accepts this flag. Clang supports
> > this flag since version 3.6[3], but the insn and builtins were only
> > added in LLVM 14[4]. I couldn't find a good solution. Should we write a
> > test to check for this insn at configuration time? Should we detect the
> > compiler at build time and emit the insns with ".long" and fixed
> > registers?
> >
> > Even building with Clang 14, the test will fail in runtime because
> > LLVM doesn't like "__int128" in inline asm. No error or warning is
> > emitted, but the generated code only loads one doubleword of the VSR.
> > The third patch of this series avoids this issue by using a vector
> > type for VSR values.
> >
> > Finally, it seems that the insns tested by
> > tests/tcg/ppc64le/byte_reverse.c are not yet supported by LLVM. Since
> > the configuration script uses '-mpower10' to check for POWER10 support
> > and Clang doesn't support this flag, "make check-tcg" doesn't fail. We
> > should probably change this check in the future, but since LLVM support
> > of POWER10 seems incomplete, I guess we can leave it for now.
>
> gitlab didn't spot any issues with the 4 patches applied.


AFAICT, CI wouldn't run into this kind of problem because we don't have 
PPC runners, and the cross-compiler containers use GCC.

> Should we merge all patches :
>
>    Use long endian options for ppc64
>    tests/tcg/ppc64le: Use vector types instead of __int128
>    target/ppc: change xs[n]madd[am]sp to use float64r32_muladd
>    tests/tcg/ppc64le: use inline asm instead of __builtin_mtfsf
>
> and see how we can address the LLVM support for P10 later ?

The problems with bcdsub.c are not resolved for Clang < 14, but I guess 
it's ok to merge anyway.

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>