Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern

[Uros Bizjak <ubizjak at gmail dot com>]

On Fri, Oct 13, 2017 at 7:30 PM, Jeff Law <law@redhat.com> wrote:
> On 09/05/2017 07:50 AM, Uros Bizjak wrote:
>> Revised patch, incorporates fixes from Alexander's review comments.
>>
>> I removed some implementation details from Alexander's description of
>> memory_blockage named pattern.
>>
>>
>> 2017-09-05  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     * target-insns.def: Add memory_blockage.
>>     * optabs.c (expand_memory_blockage): New function.
>>     (expand_asm_memory_barrier): Rename ...
>>     (expand_asm_memory_blockage): ... to this.
>>     (expand_mem_thread_fence): Call expand_memory_blockage
>>     instead of expand_asm_memory_barrier.
>>     (expand_mem_singnal_fence): Ditto.
>>     (expand_atomic_load): Ditto.
>>     (expand_atomic_store): Ditto.
>>     * doc/md.texi (Standard Pattern Names For Generation):
>>     Document memory_blockage instruction pattern.
>>
>> Bootstrapped and regression tested together with a followup x86 patch
>> on x86_64-linux-gnu {,-m32}.
>>
>> OK for mainline?
> SO I don't see anything technically wrong here.  But what I don't
> understand is the value in providing another way to do the same thing.
>
> I see a patch that introduces calls to gen_memory_blockage in the x86
> backend.  But what I don't see is why those couldn't just be
> expand_asm_memory_barrier?
>
> Were you planning a x86 specific expander?  If so what advantages does
> it have over the asm-style variant?
>
> I feel like I'm missing something here.

Yes: Alexander's patch generates asm-style blockage instruction in the
generic expansion part. If we want to include this instruction in the
peephole2 sequence, we need a different (simple and deterministic)
instruction with known (simple) RTL pattern. Proposed middle end patch
allows us to emit target-dependant UNSPEC pattern instead of a generic
asm-style pattern for the blockage instruction. This way, we can
include target-dependent UNSPEC in peephole2 sequence, as shown in a
follow-up target patch.

Uros.