Le 28/07/2015 15:55, Samuel Thibault a écrit :
> Hello,
>
> Paul Hargrove, le Mon 20 Jul 2015 23:12:10 -0700, a écrit :
>> I believe the following inline x86 asm is correct and more robust than the
>> existing code that pgi appears to reject:
> Indeed, in the 32bit case, we don't need to shuffle between 32 and 64bit
> values, so it's simpler to just use a register. It's surprising that
> letting the compiler decide the register fails more than just specifying
> SD, but since wide testing shows that, then let's go with it.
My main concern about all this is that we talked about changing "m" into
"SD" but Paul's patch did much more than that:
--- a/include/private/cpuid-x86.h
+++ b/include/private/cpuid-x86.h
@@ -72,14 +72,12 @@ static __hwloc_inline void hwloc_x86_cpuid(unsigned *eax,
unsigned *ebx, unsigne
: "+a" (*eax), "=m" (*ebx), "=&r"(sav_rbx),
"+c" (*ecx), "=&d" (*edx));
#elif defined(HWLOC_X86_32_ARCH)
- unsigned long sav_ebx;
__asm__(
- "mov %%ebx,%2\n\t"
+ "xchg %%ebx,%1\n\t"
"cpuid\n\t"
- "xchg %2,%%ebx\n\t"
- "movl %k2,%1\n\t"
- : "+a" (*eax), "=m" (*ebx), "=&r"(sav_ebx),
- "+c" (*ecx), "=&d" (*edx));
+ "xchg %%ebx,%1\n\t"
+ : "=a" (*eax), "=SD" (*ebx), "=c" (*ecx), "=d" (*edx)
+ : "0" (*eax), "2" (*ecx));
#else
#error unknown architecture
#endif
and your commit is slightly different: (s/xchg/mov/ and removed last line).
--- a/include/private/cpuid-x86.h
+++ b/include/private/cpuid-x86.h
@@ -72,14 +72,11 @@ static __hwloc_inline void hwloc_x86_cpuid(unsigned *eax,
unsigned *ebx, unsigne
: "+a" (*eax), "=m" (*ebx), "=&r"(sav_rbx),
"+c" (*ecx), "=&d" (*edx));
#elif defined(HWLOC_X86_32_ARCH)
- unsigned long sav_ebx;
__asm__(
- "mov %%ebx,%2\n\t"
+ "mov %%ebx,%1\n\t"
"cpuid\n\t"
- "xchg %2,%%ebx\n\t"
- "movl %k2,%1\n\t"
- : "+a" (*eax), "=m" (*ebx), "=&r"(sav_ebx),
- "+c" (*ecx), "=&d" (*edx));
+ "xchg %%ebx,%1\n\t"
+ : "+a" (*eax), "=SD" (*ebx), "+c" (*ecx), "=d" (*edx));
#else
#error unknown architecture
#endif
Without much explanation in git log, the history of all these fragile
asm changes becomes quite hard to read for normal people :/
My regression testing is happy so far, but it would be nice if Paul
could check again.
> I'm however afraid that this code has again posed problem, even if we
> do test its compilation in configure.ac. I'm wondering: instead of
> insisting on inlining this function, we should perhaps just put it in a
> separate .c file, which we try to compile from configure.ac exactly the
> same way as it will be for libhwloc.so?
>
FWIW, in master we don't have multiple inlining anymore (there's a
wrapper function calling this inline asm).
Brice
