[v2,4/6] i386: Remove bogus THREAD_ATOMIC_* macros

  The x86 defines optimized THREAD_ATOMIC_* macros where reference always
the current thread instead of the one indicated by input 'descr' argument.
It work as long the input is the self thread pointer, however it generates
wrong code is the semantic is to set a bit atomicialy from another thread.

This is not an issue for current GLIBC usage, however the new cancellation
code expects that some synchronization code to atomically set bits from
different threads.

If some usage indeed proves to be a hotspot we can add an extra macro
with a more descriptive name (THREAD_ATOMIC_BIT_SET_SELF for instance)
where i386 might optimize it.

Checked on i686-linux-gnu.

	* sysdeps/i686/nptl/tls.h (THREAD_ATOMIC_CMPXCHG_VAL,
	THREAD_ATOMIC_AND, THREAD_ATOMIC_BIT_SET): Remove macros.
---
 ChangeLog               |  3 +++
 sysdeps/i386/nptl/tls.h | 37 -------------------------------------
 2 files changed, 3 insertions(+), 37 deletions(-)
  

On 28/12/18 6:32 AM, Adhemerval Zanella wrote:
> The x86 defines optimized THREAD_ATOMIC_* macros where reference always
> the current thread instead of the one indicated by input 'descr' argument.
> It work as long the input is the self thread pointer, however it generates
> wrong code is the semantic is to set a bit atomicialy from another thread.

"if the semantic is to set a bit atomically"

> 
> This is not an issue for current GLIBC usage, however the new cancellation
> code expects that some synchronization code to atomically set bits from
> different threads.
> 
> If some usage indeed proves to be a hotspot we can add an extra macro
> with a more descriptive name (THREAD_ATOMIC_BIT_SET_SELF for instance)
> where i386 might optimize it.
> 
> Checked on i686-linux-gnu.
> 
> 	* sysdeps/i686/nptl/tls.h (THREAD_ATOMIC_CMPXCHG_VAL,
> 	THREAD_ATOMIC_AND, THREAD_ATOMIC_BIT_SET): Remove macros.

OK with that nit.

Siddhesh
  

* Adhemerval Zanella:

> The x86 defines optimized THREAD_ATOMIC_* macros where reference always
> the current thread instead of the one indicated by input 'descr' argument.
> It work as long the input is the self thread pointer, however it generates
> wrong code is the semantic is to set a bit atomicialy from another thread.

The entire set of THREAD_* macros is poorly documented, but this is part
of the interface: On some targets, it is expensive to obtain the thread
pointer, so it makes sense to cache it.  On other targets, including
x86-64 or i386, access to the thread pointer is implicit (or can be
cached by the compiler), so an explicit cached descriptor is
unnecessary.

I think we can improve this a lot: We could use a new type for the
cache, to make clear it is a cache, and make the macros type-safe.  Or
we could try to have the compiler cache the value for us, like we do
with __errno_location.

Thanks,
Florian
  

On 25/01/2019 13:16, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> The x86 defines optimized THREAD_ATOMIC_* macros where reference always
>> the current thread instead of the one indicated by input 'descr' argument.
>> It work as long the input is the self thread pointer, however it generates
>> wrong code is the semantic is to set a bit atomicialy from another thread.
> 
> The entire set of THREAD_* macros is poorly documented, but this is part
> of the interface: On some targets, it is expensive to obtain the thread
> pointer, so it makes sense to cache it.  On other targets, including
> x86-64 or i386, access to the thread pointer is implicit (or can be
> cached by the compiler), so an explicit cached descriptor is
> unnecessary.
> 
> I think we can improve this a lot: We could use a new type for the
> cache, to make clear it is a cache, and make the macros type-safe.  Or
> we could try to have the compiler cache the value for us, like we do
> with __errno_location.
> 
> Thanks,
> Florian
> 

It seems that besides the attribute const annotation for __errno_location,
gcc does some conditional dead call elimination (gcc/tree-call-cdce.c) to
optimize errno handling. Are you suggesting some similar? 

It also seems that some targets support __builtin_thread_pointer, maybe
this might be better option (it might simplify compiler analysis). And 
I am not sure how compiler will handle when it is based on kernel ABI.

Anyway, I agree that adding some type-safety, document the THREAD_* macros,
evaluate if arch-specific do yield better code-gen, and check for
inconsistency is indeed a gain.
  

* Adhemerval Zanella:

> On 25/01/2019 13:16, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> The x86 defines optimized THREAD_ATOMIC_* macros where reference always
>>> the current thread instead of the one indicated by input 'descr' argument.
>>> It work as long the input is the self thread pointer, however it generates
>>> wrong code is the semantic is to set a bit atomicialy from another thread.
>> 
>> The entire set of THREAD_* macros is poorly documented, but this is part
>> of the interface: On some targets, it is expensive to obtain the thread
>> pointer, so it makes sense to cache it.  On other targets, including
>> x86-64 or i386, access to the thread pointer is implicit (or can be
>> cached by the compiler), so an explicit cached descriptor is
>> unnecessary.
>> 
>> I think we can improve this a lot: We could use a new type for the
>> cache, to make clear it is a cache, and make the macros type-safe.  Or
>> we could try to have the compiler cache the value for us, like we do
>> with __errno_location.
>> 
>> Thanks,
>> Florian
>> 
>
> It seems that besides the attribute const annotation for __errno_location,
> gcc does some conditional dead call elimination (gcc/tree-call-cdce.c) to
> optimize errno handling. Are you suggesting some similar?

Yes, that was my idea.  But if GCC hard-codes the name of
__errno_location, that's probably not possible to do.

> It also seems that some targets support __builtin_thread_pointer, maybe
> this might be better option (it might simplify compiler analysis). And 
> I am not sure how compiler will handle when it is based on kernel ABI.

It would actually need Richard Henderson's namespace patches because
some architectures use something close to that (x86 for instance).
Getting the thread pointer on these architectures requires a load of a
TLS variable.

Thanks,
Florian