public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
@ 2017-09-05 13:50 Uros Bizjak
  2017-09-05 14:22 ` Alexander Monakov
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Uros Bizjak @ 2017-09-05 13:50 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: gcc-patches, Jeff Law

[-- Attachment #1: Type: text/plain, Size: 850 bytes --]

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?

Uros.

[-- Attachment #2: r.diff.txt --]
[-- Type: text/plain, Size: 4805 bytes --]

diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
index 14aab9474bc2..c4c113850fe1 100644
--- a/gcc/doc/md.texi
+++ b/gcc/doc/md.texi
@@ -6734,6 +6734,15 @@ 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{memory_blockage} instruction pattern
+@item @samp{memory_blockage}
+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.  If this pattern is not defined,
+the compiler falls back to emitting an instruction corresponding
+to @code{asm volatile ("" ::: "memory")}.
+
 @cindex @code{memory_barrier} instruction pattern
 @item @samp{memory_barrier}
 If the target memory model is not fully synchronous, then this pattern
diff --git a/gcc/optabs.c b/gcc/optabs.c
index b65707080eee..94060036e61f 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -6276,10 +6276,10 @@ expand_atomic_compare_and_swap (rtx *ptarget_bool, rtx *ptarget_oval,
   return true;
 }
 
-/* Generate asm volatile("" : : : "memory") as the memory barrier.  */
+/* Generate asm volatile("" : : : "memory") as the memory blockage.  */
 
 static void
-expand_asm_memory_barrier (void)
+expand_asm_memory_blockage (void)
 {
   rtx asm_op, clob;
 
@@ -6295,6 +6295,17 @@ expand_asm_memory_barrier (void)
   emit_insn (gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, asm_op, clob)));
 }
 
+/* Do not propagate memory accesses across this point.  */
+
+static void
+expand_memory_blockage (void)
+{
+  if (targetm.have_memory_blockage)
+    emit_insn (gen_memory_blockage ());
+  else
+    expand_asm_memory_blockage ();
+}
+
 /* This routine will either emit the mem_thread_fence pattern or issue a 
    sync_synchronize to generate a fence for memory model MEMMODEL.  */
 
@@ -6306,14 +6317,14 @@ expand_mem_thread_fence (enum memmodel model)
   if (targetm.have_mem_thread_fence ())
     {
       emit_insn (targetm.gen_mem_thread_fence (GEN_INT (model)));
-      expand_asm_memory_barrier ();
+      expand_memory_blockage ();
     }
   else if (targetm.have_memory_barrier ())
     emit_insn (targetm.gen_memory_barrier ());
   else if (synchronize_libfunc != NULL_RTX)
     emit_library_call (synchronize_libfunc, LCT_NORMAL, VOIDmode);
   else
-    expand_asm_memory_barrier ();
+    expand_memory_blockage ();
 }
 
 /* Emit a signal fence with given memory model.  */
@@ -6324,7 +6335,7 @@ expand_mem_signal_fence (enum memmodel model)
   /* No machine barrier is required to implement a signal fence, but
      a compiler memory barrier must be issued, except for relaxed MM.  */
   if (!is_mm_relaxed (model))
-    expand_asm_memory_barrier ();
+    expand_memory_blockage ();
 }
 
 /* This function expands the atomic load operation:
@@ -6346,7 +6357,7 @@ expand_atomic_load (rtx target, rtx mem, enum memmodel model)
       struct expand_operand ops[3];
       rtx_insn *last = get_last_insn ();
       if (is_mm_seq_cst (model))
-	expand_asm_memory_barrier ();
+	expand_memory_blockage ();
 
       create_output_operand (&ops[0], target, mode);
       create_fixed_operand (&ops[1], mem);
@@ -6354,7 +6365,7 @@ expand_atomic_load (rtx target, rtx mem, enum memmodel model)
       if (maybe_expand_insn (icode, 3, ops))
 	{
 	  if (!is_mm_relaxed (model))
-	    expand_asm_memory_barrier ();
+	    expand_memory_blockage ();
 	  return ops[0].value;
 	}
       delete_insns_since (last);
@@ -6404,14 +6415,14 @@ expand_atomic_store (rtx mem, rtx val, enum memmodel model, bool use_release)
     {
       rtx_insn *last = get_last_insn ();
       if (!is_mm_relaxed (model))
-	expand_asm_memory_barrier ();
+	expand_memory_blockage ();
       create_fixed_operand (&ops[0], mem);
       create_input_operand (&ops[1], val, mode);
       create_integer_operand (&ops[2], model);
       if (maybe_expand_insn (icode, 3, ops))
 	{
 	  if (is_mm_seq_cst (model))
-	    expand_asm_memory_barrier ();
+	    expand_memory_blockage ();
 	  return const0_rtx;
 	}
       delete_insns_since (last);
diff --git a/gcc/target-insns.def b/gcc/target-insns.def
index 4669439c7e1d..75976b2f8d99 100644
--- a/gcc/target-insns.def
+++ b/gcc/target-insns.def
@@ -60,6 +60,7 @@ DEF_TARGET_INSN (jump, (rtx x0))
 DEF_TARGET_INSN (load_multiple, (rtx x0, rtx x1, rtx x2))
 DEF_TARGET_INSN (mem_thread_fence, (rtx x0))
 DEF_TARGET_INSN (memory_barrier, (void))
+DEF_TARGET_INSN (memory_blockage, (void))
 DEF_TARGET_INSN (movstr, (rtx x0, rtx x1, rtx x2))
 DEF_TARGET_INSN (nonlocal_goto, (rtx x0, rtx x1, rtx x2, rtx x3))
 DEF_TARGET_INSN (nonlocal_goto_receiver, (void))

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-09-05 13:50 [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern Uros Bizjak
@ 2017-09-05 14:22 ` Alexander Monakov
  2017-09-18 21:06 ` Uros Bizjak
  2017-10-13 18:04 ` Jeff Law
  2 siblings, 0 replies; 13+ messages in thread
From: Alexander Monakov @ 2017-09-05 14:22 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: gcc-patches, Jeff Law

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-09-05 13:50 [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern Uros Bizjak
  2017-09-05 14:22 ` Alexander Monakov
@ 2017-09-18 21:06 ` Uros Bizjak
  2017-10-14 10:32   ` Andrew Pinski
  2017-10-13 18:04 ` Jeff Law
  2 siblings, 1 reply; 13+ messages in thread
From: Uros Bizjak @ 2017-09-18 21:06 UTC (permalink / raw)
  To: gcc-patches; +Cc: Jeff Law

On Tue, Sep 5, 2017 at 3:50 PM, Uros Bizjak <ubizjak@gmail.com> 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?

PING, original patch at [1].

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

Uros.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-09-05 13:50 [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern Uros Bizjak
  2017-09-05 14:22 ` Alexander Monakov
  2017-09-18 21:06 ` Uros Bizjak
@ 2017-10-13 18:04 ` Jeff Law
  2017-10-13 18:29   ` Uros Bizjak
  2 siblings, 1 reply; 13+ messages in thread
From: Jeff Law @ 2017-10-13 18:04 UTC (permalink / raw)
  To: Uros Bizjak, Alexander Monakov; +Cc: gcc-patches

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.

jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-10-13 18:04 ` Jeff Law
@ 2017-10-13 18:29   ` Uros Bizjak
  2017-10-13 18:33     ` Jeff Law
  0 siblings, 1 reply; 13+ messages in thread
From: Uros Bizjak @ 2017-10-13 18:29 UTC (permalink / raw)
  To: Jeff Law; +Cc: Alexander Monakov, gcc-patches

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-10-13 18:29   ` Uros Bizjak
@ 2017-10-13 18:33     ` Jeff Law
  0 siblings, 0 replies; 13+ messages in thread
From: Jeff Law @ 2017-10-13 18:33 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: Alexander Monakov, gcc-patches

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

OK for the trunk.

jeff

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-09-18 21:06 ` Uros Bizjak
@ 2017-10-14 10:32   ` Andrew Pinski
  2017-10-14 10:39     ` Christophe Lyon
  2017-10-14 12:33     ` Uros Bizjak
  0 siblings, 2 replies; 13+ messages in thread
From: Andrew Pinski @ 2017-10-14 10:32 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: gcc-patches, Jeff Law

On Mon, Sep 18, 2017 at 2:06 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Sep 5, 2017 at 3:50 PM, Uros Bizjak <ubizjak@gmail.com> 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?
>
> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-10-14 10:32   ` Andrew Pinski
@ 2017-10-14 10:39     ` Christophe Lyon
  2017-10-14 12:33     ` Uros Bizjak
  1 sibling, 0 replies; 13+ messages in thread
From: Christophe Lyon @ 2017-10-14 10:39 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: Uros Bizjak, gcc-patches, Jeff Law

On 14 October 2017 at 12:16, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, Sep 18, 2017 at 2:06 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Tue, Sep 5, 2017 at 3:50 PM, Uros Bizjak <ubizjak@gmail.com> 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?
>>
>> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-10-14 10:32   ` Andrew Pinski
  2017-10-14 10:39     ` Christophe Lyon
@ 2017-10-14 12:33     ` Uros Bizjak
  1 sibling, 0 replies; 13+ messages in thread
From: Uros Bizjak @ 2017-10-14 12:33 UTC (permalink / raw)
  To: Andrew Pinski; +Cc: gcc-patches, Jeff Law

On Sat, Oct 14, 2017 at 12:16 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Mon, Sep 18, 2017 at 2:06 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Tue, Sep 5, 2017 at 3:50 PM, Uros Bizjak <ubizjak@gmail.com> 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?
>>
>> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-10-14 18:28 ` Uros Bizjak
@ 2017-10-14 18:28   ` David Edelsohn
  0 siblings, 0 replies; 13+ messages in thread
From: David Edelsohn @ 2017-10-14 18:28 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: GCC Patches

On Sat, Oct 14, 2017 at 1:29 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Sat, Oct 14, 2017 at 5:44 PM, David Edelsohn <dje.gcc@gmail.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-10-14 16:00 David Edelsohn
  2017-10-14 16:29 ` David Edelsohn
@ 2017-10-14 18:28 ` Uros Bizjak
  2017-10-14 18:28   ` David Edelsohn
  1 sibling, 1 reply; 13+ messages in thread
From: Uros Bizjak @ 2017-10-14 18:28 UTC (permalink / raw)
  To: David Edelsohn; +Cc: GCC Patches

On Sat, Oct 14, 2017 at 5:44 PM, David Edelsohn <dje.gcc@gmail.com> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
  2017-10-14 16:00 David Edelsohn
@ 2017-10-14 16:29 ` David Edelsohn
  2017-10-14 18:28 ` Uros Bizjak
  1 sibling, 0 replies; 13+ messages in thread
From: David Edelsohn @ 2017-10-14 16:29 UTC (permalink / raw)
  To: Uros Bizjak, Segher Boessenkool, Bill Seurer, William J. Schmidt
  Cc: GCC Patches

On Sat, Oct 14, 2017 at 11:44 AM, David Edelsohn <dje.gcc@gmail.com> 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 <https://gcc.gnu.org/bugs/> for instructions.
make: *** [emutls.o] Error 1

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern
@ 2017-10-14 16:00 David Edelsohn
  2017-10-14 16:29 ` David Edelsohn
  2017-10-14 18:28 ` Uros Bizjak
  0 siblings, 2 replies; 13+ messages in thread
From: David Edelsohn @ 2017-10-14 16:00 UTC (permalink / raw)
  To: Uros Bizjak; +Cc: GCC Patches

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-10-14 18:28 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-05 13:50 [PATCH v2, middle-end]: Introduce memory_blockage named insn pattern Uros Bizjak
2017-09-05 14:22 ` Alexander Monakov
2017-09-18 21:06 ` Uros Bizjak
2017-10-14 10:32   ` Andrew Pinski
2017-10-14 10:39     ` Christophe Lyon
2017-10-14 12:33     ` Uros Bizjak
2017-10-13 18:04 ` Jeff Law
2017-10-13 18:29   ` Uros Bizjak
2017-10-13 18:33     ` Jeff Law
2017-10-14 16:00 David Edelsohn
2017-10-14 16:29 ` David Edelsohn
2017-10-14 18:28 ` Uros Bizjak
2017-10-14 18:28   ` David Edelsohn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).