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

2017-10-14 Thread David Edelsohn 
On Sat, Oct 14, 2017 at 1:29 PM, Uros Bizjak  wrote:
> On Sat, Oct 14, 2017 at 5:44 PM, David Edelsohn  wrote:
>> This patch has broken bootstrap on AIX and possibly powerpc64-linux.
>> Was this patch tested on any architecture other than x86?
>
> No.
>
>> /nasfarm/edelsohn/src/src/libgcc/emutls.c: In function 
>> '__emutls_get_address':
>> /nasfarm/edelsohn/src/src/libgcc/emutls.c:139:11: internal compiler
>> error: in invalid_void, at config/rs6000/rs6000.md:10804
>>pointer offset = __atomic_load_n (&obj->loc.offset, __ATOMIC_ACQUIRE);
>>^~
>
> Basically, the patch does only:
>
> +expand_memory_blockage (void)
> +{
> +  if (targetm.have_memory_blockage)
> +emit_insn (targetm.gen_memory_blockage ());
> +  else
> +expand_asm_memory_blockage ();
> +}
>
> So, if the target doesn't declare memory_blockage pattern, as is the
> case with rs6000, I really fail to see what could go wrong here.

Thanks for finding and fixing the targetm.have_memory_blockage typo.

- David

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

2017-10-14 Thread Uros Bizjak 
On Sat, Oct 14, 2017 at 5:44 PM, David Edelsohn  wrote:
> This patch has broken bootstrap on AIX and possibly powerpc64-linux.
> Was this patch tested on any architecture other than x86?

No.

> /nasfarm/edelsohn/src/src/libgcc/emutls.c: In function '__emutls_get_address':
> /nasfarm/edelsohn/src/src/libgcc/emutls.c:139:11: internal compiler
> error: in invalid_void, at config/rs6000/rs6000.md:10804
>pointer offset = __atomic_load_n (&obj->loc.offset, __ATOMIC_ACQUIRE);
>^~

Basically, the patch does only:

+expand_memory_blockage (void)
+{
+  if (targetm.have_memory_blockage)
+emit_insn (targetm.gen_memory_blockage ());
+  else
+expand_asm_memory_blockage ();
+}

So, if the target doesn't declare memory_blockage pattern, as is the
case with rs6000, I really fail to see what could go wrong here.

Uros.

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

2017-10-14 Thread David Edelsohn 
On Sat, Oct 14, 2017 at 11:44 AM, David Edelsohn  wrote:
> This patch has broken bootstrap on AIX and possibly powerpc64-linux.
> Was this patch tested on any architecture other than x86?
>
> /nasfarm/edelsohn/src/src/libgcc/emutls.c: In function '__emutls_get_address':
> /nasfarm/edelsohn/src/src/libgcc/emutls.c:139:11: internal compiler
> error: in invalid_void, at config/rs6000/rs6000.md:10804
>pointer offset = __atomic_load_n (&obj->loc.offset, __ATOMIC_ACQUIRE);
>^~

Same failure on powerpc64-linux using the GNU Compile Farm

during RTL pass: expand
/home/dje/src/src/libgcc/emutls.c: In function ‘__emutls_get_address’:
/home/dje/src/src/libgcc/emutls.c:139:11: internal compiler error: in
invalid_void, at config/rs6000/rs6000.md:10804
   pointer offset = __atomic_load_n (&obj->loc.offset, __ATOMIC_ACQUIRE);
   ^~

0x11401a13 invalid_void
/home/dje/src/src/gcc/config/rs6000/rs6000.md:10804
0x10c22263 expand_memory_blockage
/home/dje/src/src/gcc/optabs.c:6301
0x10c225cb expand_atomic_load(rtx_def*, rtx_def*, memmodel)
/home/dje/src/src/gcc/optabs.c:6365
0x1056a14b expand_builtin_atomic_load
/home/dje/src/src/gcc/builtins.c:5951
0x1056fd87 expand_builtin(tree_node*, rtx_def*, rtx_def*, machine_mode, int)
/home/dje/src/src/gcc/builtins.c:7280
0x107ecf9f expand_expr_real_1(tree_node*, rtx_def*, machine_mode,
expand_modifier, rtx_def**, bool)
/home/dje/src/src/gcc/expr.c:10866
0x107de5c7 expand_expr_real(tree_node*, rtx_def*, machine_mode,
expand_modifier, rtx_def**, bool)
/home/dje/src/src/gcc/expr.c:8084
0x107d2d9b store_expr_with_bounds(tree_node*, rtx_def*, int, bool,
bool, tree_node*)
/home/dje/src/src/gcc/expr.c:5554
0x107d14e3 expand_assignment(tree_node*, tree_node*, bool)
/home/dje/src/src/gcc/expr.c:5319
0x105bb9c3 expand_call_stmt
/home/dje/src/src/gcc/cfgexpand.c:2664
0x105bf95b expand_gimple_stmt_1
/home/dje/src/src/gcc/cfgexpand.c:3585
0x105c025b expand_gimple_stmt
/home/dje/src/src/gcc/cfgexpand.c:3751
0x105c9dbf expand_gimple_basic_block
/home/dje/src/src/gcc/cfgexpand.c:5754
0x105cbf73 execute
/home/dje/src/src/gcc/cfgexpand.c:6361
Please submit a full bug report,
with preprocessed source if appropriate.
Please include the complete backtrace with any bug report.
See  for instructions.
make: *** [emutls.o] Error 1

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

2017-10-14 Thread David Edelsohn 
This patch has broken bootstrap on AIX and possibly powerpc64-linux.
Was this patch tested on any architecture other than x86?

/nasfarm/edelsohn/src/src/libgcc/emutls.c: In function '__emutls_get_address':
/nasfarm/edelsohn/src/src/libgcc/emutls.c:139:11: internal compiler
error: in invalid_void, at config/rs6000/rs6000.md:10804
   pointer offset = __atomic_load_n (&obj->loc.offset, __ATOMIC_ACQUIRE);
   ^~

Thanks, David

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

2017-10-14 Thread Uros Bizjak 
On Sat, Oct 14, 2017 at 12:16 PM, Andrew Pinski  wrote:
> On Mon, Sep 18, 2017 at 2:06 PM, Uros Bizjak  wrote:
>> On Tue, Sep 5, 2017 at 3:50 PM, 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  
>>>
>>> * 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?
>>
>> PING, original patch at [1].
>>
>> [1] https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00270.html
>
> This patch broke aarch64-linux-gnu and aarch64-elf building:
>
> ./.deps/optabs-query.TPo
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs-query.c
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs.c:
> In function ‘void expand_memory_blockage()’:
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs.c:6301:
> error: ‘gen_memory_blockage’ was not declared in this scope

Will fix ASAP:

Index: optabs.c
===
--- optabs.c(revision 253750)
+++ optabs.c(working copy)
@@ -6298,7 +6298,7 @@ static void
 expand_memory_blockage (void)
 {
   if (targetm.have_memory_blockage)
-emit_insn (gen_memory_blockage ());
+emit_insn (targetm.gen_memory_blockage ());
   else
 expand_asm_memory_blockage ();
 }

Uros.

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

2017-10-14 Thread Christophe Lyon 
On 14 October 2017 at 12:16, Andrew Pinski  wrote:
> On Mon, Sep 18, 2017 at 2:06 PM, Uros Bizjak  wrote:
>> On Tue, Sep 5, 2017 at 3:50 PM, 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  
>>>
>>> * 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?
>>
>> PING, original patch at [1].
>>
>> [1] https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00270.html
>
> This patch broke aarch64-linux-gnu and aarch64-elf building:
>
> ./.deps/optabs-query.TPo
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs-query.c
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs.c:
> In function ‘void expand_memory_blockage()’:
> /home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs.c:6301:
> error: ‘gen_memory_blockage’ was not declared in this scope
>

Same error for arm.

Christophe

> Thanks,
> Andrew
>
>
>>
>> Uros.

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

2017-10-14 Thread Andrew Pinski 
On Mon, Sep 18, 2017 at 2:06 PM, Uros Bizjak  wrote:
> On Tue, Sep 5, 2017 at 3:50 PM, 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  
>>
>> * 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?
>
> PING, original patch at [1].
>
> [1] https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00270.html

This patch broke aarch64-linux-gnu and aarch64-elf building:

./.deps/optabs-query.TPo
/home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs-query.c
/home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs.c:
In function ‘void expand_memory_blockage()’:
/home/jenkins/workspace/BuildToolchainAARCH64_thunder_elf_upstream/toolchain/scripts/../src/gcc/optabs.c:6301:
error: ‘gen_memory_blockage’ was not declared in this scope

Thanks,
Andrew


>
> Uros.

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

2017-10-13 Thread Jeff Law 
On 10/13/2017 12:27 PM, Uros Bizjak wrote:
> On Fri, Oct 13, 2017 at 7:30 PM, Jeff Law  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  
>>>
>>> * 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.
Ah, you want to use it as a sequence within a peep2 match.  That makes
sense now.

OK for the trunk.

jeff

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

2017-10-13 Thread Uros Bizjak 
On Fri, Oct 13, 2017 at 7:30 PM, Jeff Law  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  
>>
>> * 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.

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

2017-10-13 Thread Jeff Law 
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  
> 
> * 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.

jeff

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

2017-09-18 Thread Uros Bizjak 
On Tue, Sep 5, 2017 at 3:50 PM, 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  
>
> * 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?

PING, original patch at [1].

[1] https://gcc.gnu.org/ml/gcc-patches/2017-09/msg00270.html

Uros.

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

2017-09-05 Thread Alexander Monakov 
On Tue, 5 Sep 2017, 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.

Well, to me it wasn't really obvious why a named pattern was needed
in the first place, so I wish the explanation could stay in some form.


One small nit, the new function comment in optabs.c,

+/* Do not propagate memory accesses across this point.  */

doesn't seem appropriate, it should probably say something like

/* Emit an insn acting as a compiler memory barrier.  */

Rest of the patch looks fine to me (but I cannot approve it).

Thanks.
Alexander