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

Richard Sandiford richard@codesourcery.com
Sun Sep 9 11:00:00 GMT 2007


David Daney <ddaney@avtrex.com> writes:
> Drat, I miss-addressed this when I first sent it.  Here it is again...
>> The mips port is currently FAILing the libjava PR27908 test case.  
>> This is a test of compliance with the java memory model where a read 
>> from a volatile field must be a memory barrier causing all subsequent 
>> access to  be synchronized with memory (I hope that makes sense).  The 
>> documentation for the atomic memory operations hint at this memory 
>> barrier behavior, but it was unclear enough that in the original 
>> atomic memory operations patch, I didn't have memory clobbers.
>>
>> Thinking about it more it seems to me that the only way it makes sense 
>> to have these atomic memory builtins is if they clobber not only the 
>> locations they are synchronizing, but also all of memory.  There is a 
>> reason that all the ad hoc in-line assembly primitives that do similar 
>> operations clobber memory.  The GCC builtins need to do it as well.
>>
>> This patch adds memory clobbers to the MIPS atomic memory operations.  
>> After examining the assembly from the libjava PR27908 test case, I 
>> think this fixes the problem.  But real testing will be the real test...
>>
>> Currently testing on mipsel-linux.
>>
>> OK to commit if it passes?

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.  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.

If:

Index: config/mips/mips.md
===================================================================
--- config/mips/mips.md	(revision 128245)
+++ config/mips/mips.md	(working copy)
@@ -57,6 +57,7 @@ (define_constants
    (UNSPEC_SYNC_OLD_OP		38)
    (UNSPEC_SYNC_NEW_OP		39)
    (UNSPEC_SYNC_EXCHANGE	40)
+   (UNSPEC_MEMORY_BARRIER	41)
    
    (UNSPEC_ADDRESS_FIRST	100)
 
@@ -4304,10 +4305,11 @@ (define_insn "clear_hazard"
 
 ;; Atomic memory operations.
 
-(define_expand "memory_barrier"
-  [(unspec_volatile [(const_int 0)] UNSPEC_SYNC)]
+(define_insn "memory_barrier"
+  [(set (mem:BLK (scratch))
+        (unspec:BLK [(const_int 0)] UNSPEC_MEMORY_BARRIER))]
   "ISA_HAS_SYNC"
-  "")
+  "sync")
 
 (define_insn "sync_compare_and_swap<mode>"
   [(set (match_operand:GPR 0 "register_operand" "=&d,d")

works, then it's OK, thanks.

Richard



More information about the Gcc-patches mailing list