On Tue, Sep 5, 2017 at 2:03 PM, Alexander Monakov <amona...@ispras.ru> wrote:
> 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).
Uh, I was not aware of the new approach. Will send a v2 with mentioned
issues fixed.
Thanks,
Uros.
