[Patch] MIPS: Add missing memory clobbers to atomic memory insns.

David Daney ddaney@avtrex.com
Sun Sep 9 17:58:00 GMT 2007


Richard Sandiford wrote:
> The idea looks sound, but...
>
>   
>> -(define_expand "memory_barrier"
>> -  [(unspec_volatile [(const_int 0)] UNSPEC_SYNC)]
>> +(define_insn "memory_barrier"
>> +  [(set (mem:BLK (scratch))
>> +        (unspec_volatile:BLK [(const_int 0)] UNSPEC_MEMORY_BARRIER))]
>>    "ISA_HAS_SYNC"
>> -  "")
>> +  "sync")
>>     
>
> ...the insn is doing nothing other than acting as a memory barrier,
> so I don't think it needs to be unspec_volatile after this change.
> A plain unspec should do.
OK I will try that instead.

>   gcc should still add the required dependencies
> for other memory operations, but the sync really does have no dependencies
> on preceding arithmetic ops.
>
> Do all the sync loop patterns really need the clobber too?
> The memory_barrier alone should be enough.
>
>   

I think any pattern that allows us to test the result of an atomic 
operation requires a memory clobber.  Indeed gcc needs to track the 
explicit dependencies of the operations.  However, I am worried about 
the implicit dependencies that may only be known to the program author.  
Presumably there are non-volatile reads that need to be protected by the 
atomic memory operation.  The problem I was seeing with the 
memory_barrier case I think would also occur with the other operations 
as well.  For example in:

-----------
int a;
int b;
int q;

void c()
{
  while (a)
    {
      if (__sync_add_and_fetch (&b, 1) == 5)
    q++;
    }
}
-----------
What keeps loads of a and q from being hoisted outside of the loop?

That is why I wanted to add memory clobbers to everything.  Although 
correct code is being generated even without adding the clobbers.  Is 
that due to chance, or is there some mechanism that prevents it?

David Daney



More information about the Gcc-patches mailing list