On Thu, 15 Jan 2015, Jakub Jelinek wrote:
> On Thu, Jan 15, 2015 at 07:46:18AM +0100, Richard Biener wrote:
> > >> That last line means the compiler is free to delete a non-volatile
> > >> asm with a memory clobber if that asm is not needed for dataflow. Or
> > >> that is how I read it; it is trying to indicate that if you want to
> > >> prevent the memory clobber from being deleted (together with the rest
> > >> of the asm), you need to make the asm volatile.
> > >>
> > >> So as far as I can see the compiler can CSE two identical
> > >non-volatile
> > >> asms with memory clobber just fine. Older GCC (I tried 4.7.2) does
> > >do
> > >> this; current mainline doesn't. I think it should.
> > >No, it should not CSE those two cases. That's simply wrong and if an
> > >older version did that optimization, that's a bug.
> >
> > I think segher has a point here. If the asm with memory clobber would
> > store to random memory and the point would be to preserve that then the
> > whole distinction with volatile doesn't make much sense (after all without
> > volatile we happily DCE such asm if the regular outputs are not needed).
> >
> > This doesn't mean 'memory' is a well-designed thing, of course. Just its
> > effects are effectively limited to reads without volatile(?)
>
> Segher's mails talk about "memory" being a write but not read.
> If we even can't agree on what non-volatile "memory" means, I think
> we should treat it more conservatively, because every user (and there are
> lots of people using non-volatile asm with "memory" in the wild) treats it
> differently. Just trying to grep for a few:
> glibc:
> ./sysdeps/alpha/bits/atomic.h:# define atomic_full_barrier() __asm ("mb" : :
> : "memory");
> ./sysdeps/alpha/bits/atomic.h:# define atomic_read_barrier() __asm ("mb" : :
> : "memory");
> ./sysdeps/alpha/bits/atomic.h:# define atomic_write_barrier() __asm ("wmb" :
> : : "memory");
> ./sysdeps/sparc/sparc32/bits/atomic.h:# define atomic_full_barrier() __asm
> ("" ::: "memory")
> ./sysdeps/powerpc/powerpc32/bits/atomic.h:# define atomic_read_barrier()
> __asm ("lwsync" ::: "memory")
> ./sysdeps/powerpc/powerpc32/bits/atomic.h:# define atomic_read_barrier()
> __asm ("sync" ::: "memory")
> ./sysdeps/powerpc/powerpc64/bits/atomic.h:#define atomic_read_barrier()
> __asm ("lwsync" ::: "memory")
> ./sysdeps/powerpc/bits/atomic.h:#define atomic_full_barrier() __asm ("sync"
> ::: "memory")
> ./sysdeps/powerpc/bits/atomic.h:#define atomic_write_barrier() __asm
> ("eieio" ::: "memory")
> ./sysdeps/generic/malloc-machine.h:# define atomic_full_barrier() __asm (""
> ::: "memory")
> ./elf/tst-tlsmod3.c: asm ("" ::: "memory");
> ./elf/tst-tlsmod4.c: asm ("" ::: "memory");
> ./elf/tst-tlsmod1.c: asm ("" ::: "memory");
> ./elf/tst-tlsmod2.c: asm ("" ::: "memory");
> ./include/atomic.h:# define atomic_full_barrier() __asm ("" ::: "memory")
> linux kernel:
> ./arch/arm/mach-omap2/pm24xx.c: asm("mcr p15, 0, %0, c7, c0, 4" : : "r"
> (zero) : "memory", "cc");
> ./arch/arm/include/asm/irqflags.h:#define local_fiq_enable() __asm__("cpsie
> f @ __stf" : : : "memory", "cc")
> ./arch/arm/include/asm/irqflags.h:#define local_fiq_disable() __asm__("cpsid
> f @ __clf" : : : "memory", "cc")
> ./arch/x86/include/asm/uaccess_64.h: asm("":::"memory");
> ./arch/x86/include/asm/uaccess_64.h: asm("":::"memory");
> ./arch/x86/include/asm/segment.h: asm("mov %%" #seg ",%0":"=r" (value) :
> : "memory")
> ./arch/x86/include/asm/stackprotector.h: asm("mov %0, %%gs" : : "r"
> (__KERNEL_STACK_CANARY) : "memory");
> ./arch/arm64/include/asm/irqflags.h:#define local_fiq_enable()
> asm("msr daifclr, #1" : : : "memory")
> ./arch/arm64/include/asm/irqflags.h:#define local_fiq_disable()
> asm("msr daifset, #1" : : : "memory")
> ./arch/arm64/include/asm/irqflags.h:#define local_async_enable()
> asm("msr daifclr, #4" : : : "memory")
> ./arch/arm64/include/asm/irqflags.h:#define local_async_disable()
> asm("msr daifset, #4" : : : "memory")
> ./arch/tile/lib/memcpy_64.c: __asm__ ("" : : :
> "memory");
> ./arch/tile/lib/memcpy_64.c: __asm__ ("" : : :
> "memory");
> ./arch/tile/include/hv/netio_intf.h: __asm__("" : : : "memory");
> ./drivers/net/ethernet/sgi/ioc3-eth.c: __asm__("sync" ::: "memory")
> ./lib/sha1.c: #define setW(x, val) do { W(x) = (val);
> __asm__("":::"memory"); } while (0)
>
> The glibc barriers are supposedly something that can be CSEd (one barrier
> instead of
> two consecutive barriers is enough), but certainly not moved across any
> loads/stores
> in between. In the kernel case, the enable/disable probably wouldn't allow
> even CSE.
>
> So I'm with Jeff that we should treat "memory" at least as unspecified read
> and write,
> and whether we can CSE them if there are no memory loads/stores in between
> them can
> be discussed (most likely the kernel would be safe even in that case, because
> those
> usually don't nest and appear in pairs, or act as barriers (like the glibc
> case),
> or read from segment registers (guess again ok to be CSEd with no intervening
> loads/stores).
>
> In 4.9 backport I'd prefer not to CSE them at all though, stay conservative.
Sure - I also requested "memory" to be not CSEd just to be conservative,
not because I fully understood its meaning.
Richard.
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)
