[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))]
> -  "")
> +  "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.


Index: config/mips/mips.md
--- config/mips/mips.md	(revision 128245)
+++ config/mips/mips.md	(working copy)
@@ -57,6 +57,7 @@ (define_constants
@@ -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))]
-  "")
+  "sync")
 (define_insn "sync_compare_and_swap<mode>"
   [(set (match_operand:GPR 0 "register_operand" "=&d,d")

works, then it's OK, thanks.


More information about the Gcc-patches mailing list