On 12/17/18 5:50 AM, David Gibson wrote: > On Sat, Dec 15, 2018 at 07:42:22PM +0100, Cédric Le Goater wrote: >> Fix the PPC_BIT definitions to use ULL instead in UL and replace >> __builtin_ffssl() by the equivalent ctz routines. >> >> Signed-off-by: Cédric Le Goater <c...@kaod.org> >> --- >> >> Compile tested with --cross-prefix=x86_64-w64-mingw32-. When I have >> some more time, I might try runtime on windows also. > > Which version of the mingw compiler do you have? As I've noted > elsewhere, I haven't been able to reproduce the problem with the mingw > in Fedora 29.
I have : mingw64-gcc-7.3.0-1.fc28.x86_64 >> The PPC compile failures have been there for a while (pre 2.12) and >> the MASK_TO_LSH macro was not used until the XIVE definitions were >> introduced. >> >> I let you guys decide on how you want to proceed, but I think it is >> safe to merge this patch as a prereq of the pull request. >> >> Thanks, >> >> C. >> >> target/ppc/cpu.h | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h >> index ab68abe8a23c..a3d3e91eb4ce 100644 >> --- a/target/ppc/cpu.h >> +++ b/target/ppc/cpu.h >> @@ -70,18 +70,18 @@ >> #define PPC_ELF_MACHINE EM_PPC >> #endif >> >> -#define PPC_BIT(bit) (0x8000000000000000UL >> (bit)) >> -#define PPC_BIT32(bit) (0x80000000UL >> (bit)) >> -#define PPC_BIT8(bit) (0x80UL >> (bit)) >> +#define PPC_BIT(bit) (0x8000000000000000ULL >> (bit)) >> +#define PPC_BIT32(bit) (0x80000000ULL >> (bit)) >> +#define PPC_BIT8(bit) (0x80ULL >> (bit)) > > We shouldn't need the ULL for the 32-bit and (especially) 8-bit > versions. > >> #define PPC_BITMASK(bs, be) ((PPC_BIT(bs) - PPC_BIT(be)) | PPC_BIT(bs)) >> #define PPC_BITMASK32(bs, be) ((PPC_BIT32(bs) - PPC_BIT32(be)) | \ >> PPC_BIT32(bs)) >> #define PPC_BITMASK8(bs, be) ((PPC_BIT8(bs) - PPC_BIT8(be)) | >> PPC_BIT8(bs)) >> >> #if HOST_LONG_BITS == 32 >> -# define MASK_TO_LSH(m) (__builtin_ffsll(m) - 1) >> +# define MASK_TO_LSH(m) ctz32(m) >> #elif HOST_LONG_BITS == 64 >> -# define MASK_TO_LSH(m) (__builtin_ffsl(m) - 1) >> +# define MASK_TO_LSH(m) ctz64(m) > > Having another look at this, this seems bogus: we're defining an > interface that's only correct on a *host long*. But when we use it > we're almost certain to be working on guest values with a fixed size, > so using this wrapper just adds confusion. > > I'm not really seeing any reason to use MASK_TO_LSH() rather than > using ctz32() and ctz64() explicitly. Does that mean you think we should define GETFIELD32/64 and SETFIELD32/64 macros ? or should we use the extract32/64 and deposit32/64 helpers instead ? That's a bigger change but may be a better one. Also, in the definitions above, only PPC_BIT() needs to use a ULL. C.