Patchwork powerpc lack of memory_barrier

login
register
mail settings
Submitter Alan Modra
Date Nov. 17, 2011, 11:07 a.m.
Message ID <20111117110713.GZ14325@bubble.grove.modra.org>
Download mbox | patch
Permalink /patch/126184/
State New
Headers show

Comments

Alan Modra - Nov. 17, 2011, 11:07 a.m.
Lacking this pattern means the builtin __sync_synchronize() on powerpc
is just an asm with a memory clobber (see builtins.c), which is hardly
a "full memory barrier" as extend.texi says it should be.  This patch
fixes multiple libgomp testsuite failures.  Bootstrapped and
regression tested powerpc-linux.  OK for mainline?

	* config/rs6000/sync.md: Duplicate hwsync as memory_barrier.
David Edelsohn - Nov. 17, 2011, 2:50 p.m.
On Thu, Nov 17, 2011 at 6:07 AM, Alan Modra <amodra@gmail.com> wrote:
> Lacking this pattern means the builtin __sync_synchronize() on powerpc
> is just an asm with a memory clobber (see builtins.c), which is hardly
> a "full memory barrier" as extend.texi says it should be.  This patch
> fixes multiple libgomp testsuite failures.  Bootstrapped and
> regression tested powerpc-linux.  OK for mainline?
>
>        * config/rs6000/sync.md: Duplicate hwsync as memory_barrier.

This is one solution.  Alternatively,the expander for "hwsync" could
be renamed "memory_barrier" and all uses of gen_hwsync() changed to
gen_memory_barrier().

I don't mind either way, but I would like Richard' opinion.
Consistently using memory_barrier might be clearer to other GCC
developers who look at the rs6000 port.

Thanks, David
Richard Henderson - Nov. 17, 2011, 4:23 p.m.
On 11/17/2011 04:50 AM, David Edelsohn wrote:
> On Thu, Nov 17, 2011 at 6:07 AM, Alan Modra <amodra@gmail.com> wrote:
>> Lacking this pattern means the builtin __sync_synchronize() on powerpc
>> is just an asm with a memory clobber (see builtins.c), which is hardly
>> a "full memory barrier" as extend.texi says it should be.  This patch
>> fixes multiple libgomp testsuite failures.  Bootstrapped and
>> regression tested powerpc-linux.  OK for mainline?
>>
>>        * config/rs6000/sync.md: Duplicate hwsync as memory_barrier.
> 
> This is one solution.  Alternatively,the expander for "hwsync" could
> be renamed "memory_barrier" and all uses of gen_hwsync() changed to
> gen_memory_barrier().
> 
> I don't mind either way, but I would like Richard' opinion.
> Consistently using memory_barrier might be clearer to other GCC
> developers who look at the rs6000 port.

We're supposed to be looking at the new atomics patterns first, when
they are available.  Apparently __sync_synchronize got missed.  It
should be looking at mem_thread_fence before looking for memory_barrier.

Will fix.


r~

Patch

Index: gcc/config/rs6000/sync.md
===================================================================
--- gcc/config/rs6000/sync.md	(revision 181425)
+++ gcc/config/rs6000/sync.md	(working copy)
@@ -53,6 +53,15 @@  (define_expand "mem_thread_fence"
   DONE;
 })
 
+(define_expand "memory_barrier"
+  [(set (match_dup 0)
+	(unspec:BLK [(match_dup 0)] UNSPEC_SYNC))]
+  ""
+{
+  operands[0] = gen_rtx_MEM (BLKmode, gen_rtx_SCRATCH (Pmode));
+  MEM_VOLATILE_P (operands[0]) = 1;
+})
+
 (define_expand "hwsync"
   [(set (match_dup 0)
 	(unspec:BLK [(match_dup 0)] UNSPEC_SYNC))]