On Tue, 5 Sep 2017, Uros Bizjak wrote:
> However, this definition can't be generic, since unspec is used.

I see, if the only reason this needs a named pattern is lack of generic UNSPEC
values, I believe it would be helpful to mention that in the documentation.

A few comments on the patch:

> @@ -6734,6 +6734,13 @@ scheduler and other passes from moving instructions 
> and using register
>  equivalences across the boundary defined by the blockage insn.
>  This needs to be an UNSPEC_VOLATILE pattern or a volatile ASM.
>  
> +@cindex @code{memmory_blockage} instruction pattern

Typo ('mm').

> +@item @samp{memory_blockage}
> +This pattern defines a pseudo insn that prevents the instruction
> +scheduler and other passes from moving instructions accessing memory
> +across the boundary defined by the blockage insn.  This instruction
> +needs to read and write volatile BLKmode memory.
> +

I see this is mostly cloned from the 'blockage' pattern description, but
this is not quite correct, it's not about moving _instructions_ per se
(RTL CSE propagates values loaded from memory without moving instructions,
RTL DSE eliminates some stores to memory also without moving instructions),
and calling out the scheduler separately doesn't seem useful.  I suggest:

"""
This pattern, if defined, represents a compiler memory barrier, and will be
placed at points across which RTL passes may not propagate memory accesses.
This instruction needs to read and write volatile BLKmode memory.  It does
not need to generate any machine instruction, and like the @code{blockage}
insn needs a named pattern only because there are no generic @code{unspec}
values.  If this pattern is not defined, the compiler falls back by emitting
an instruction corresponding to @code{asm volatile ("" ::: "memory")}.
"""

> @@ -6295,6 +6295,21 @@ expand_asm_memory_barrier (void)
>    emit_insn (gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, asm_op, clob)));
>  }
>  
> +#ifndef HAVE_memory_blockage
> +#define HAVE_memory_blockage 0
> +#endif

Why this?  This style is not used (anymore) elsewhere in the file, afaict
the current approach is to add a definition in target-insns.def and then
use targetm.have_memory_blockage (e.g. like mem_thread_fence is used).

Thanks.
Alexander

Reply via email to