Re: Patch for PR libgomp/37938, IA64 specific.

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Thu, Nov 6, 2008 at 9:29 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Nov 6, 2008 at 9:03 AM, Steve Ellcey <sje@cup.hp.com> wrote:
>>
>> On Tue, 2008-11-04 at 18:17 +0100, Jakub Jelinek wrote:
>>
>>> Shouldn't sync_lock_release<mode> get the same treatment on ia64 though?
>>
>> I think you are right.  I have added a memory_barrier call in
>> sync_lock_release<mode> to my patch and retested.  Here is the current
>> patch I intend to check in:
>>
>> 2008-11-06  Steve Ellcey  <sje@cup.hp.com>
>>
>>        PR libgomp/37938
>>        * config/ia64/sync.md (sync_lock_test_and_set_xchg<mode>): New name
>>        for original sync_lock_test_and_set<mode>.
>>        (sync_lock_test_and_set<mode>): Create define_expand version.
>>        (sync_lock_release<mode>): Add memory_barrier.
>>
>>
>> Index: config/ia64/sync.md
>> ===================================================================
>> --- config/ia64/sync.md (revision 141635)
>> +++ config/ia64/sync.md (working copy)
>> @@ -159,7 +159,20 @@ (define_insn "cmpxchg_rel_di"
>>   "cmpxchg8.rel %0 = %1, %3, %2"
>>   [(set_attr "itanium_class" "sem")])
>>
>> -(define_insn "sync_lock_test_and_set<mode>"
>> +(define_expand "sync_lock_test_and_set<mode>"
>> +  [(set (match_operand:IMODE 0 "gr_register_operand" "")
>> +        (match_operand:IMODE 1 "not_postinc_memory_operand" ""))
>> +   (set (match_dup 1)
>> +        (match_operand:IMODE 2 "gr_register_operand" ""))]
>> +  ""
>> +{
>> +  emit_insn (gen_memory_barrier ());
>> +  emit_insn (gen_sync_lock_test_and_set_xchg<mode> (operands[0], operands[1],
>> +            operands[2]));
>> +  DONE;
>> +})
>> +
>> +(define_insn "sync_lock_test_and_set_xchg<mode>"
>>   [(set (match_operand:IMODE 0 "gr_register_operand" "=r")
>>         (match_operand:IMODE 1 "not_postinc_memory_operand" "+S"))
>>    (set (match_dup 1)
>> @@ -173,5 +186,6 @@ (define_expand "sync_lock_release<mode>"
>>        (match_operand:IMODE 1 "gr_reg_or_0_operand" ""))]
>>   ""
>>  {
>> +  emit_insn (gen_memory_barrier ());
>>   gcc_assert (MEM_VOLATILE_P (operands[0]));
>>  })
>>
>
> I checked icc 11.0. It doesn't generate "mf" for __sync_lock_test_and_set.
>
> --
> #include <ia64intrin.h>
>
> int
> foo (int *p, int x)
> {
>  return __sync_lock_test_and_set (p, x);
> }
> --
>
> generates:
>
> foo:
>        .prologue
>        .body
>        .mib
>        xchg4 r8 = [r32], r33
>        nop 0
>        br.ret.sptk.many b0
>        .endp foo#
>
> xchg4 is performed with acquire semantics, i.e., the memory read/write is made
> visible prior to all subsequent data memory access. If mf makes a difference,
> that means we may access the memory with release or unordered semantics
> before. If I have to guess, we may have an unordered read somewhere.
>
> --
> H.J.
>

Shouldn't we use __sync_lock_test_and_set  to initialize mutux?


H.J.
--- libgomp/config/linux/mutex.h.sync	2006-11-18 06:22:18.000000000 -0800
+++ libgomp/config/linux/mutex.h	2008-11-06 09:50:46.000000000 -0800
@@ -38,7 +38,7 @@ typedef int gomp_mutex_t;

 static inline void gomp_mutex_init (gomp_mutex_t *mutex)
 {
-  *mutex = 0;
+  __sync_lock_test_and_set (mutex, 0);
 }

 extern void gomp_mutex_lock_slow (gomp_mutex_t *mutex);


-- 
H.J.