Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

[Uros Bizjak]

> On 09/01/2017 12:26 AM, Alexander Monakov wrote:
>> On Thu, 31 Aug 2017, Jeff Law wrote:
>>> This is OK.
>>>
>>> What's the point of the delete_insns_since calls in patch #3?
>>
>> Deleting the first barrier when maybe_expand_insn failed.
>> Other functions in the file use a similar approach.
> Thanks for clarifying.  OK for the trunk.  Sorry about the wait.

It looks that:

2017-09-04  Alexander Monakov  <amonakov@ispras.ru>

        PR rtl-optimization/57448
        PR target/67458
        PR target/81316
        * optabs.c (expand_atomic_load): Place compiler memory barriers if
        using atomic_load pattern.
        (expand_atomic_store): Likewise.

introduced a couple of regressions on x86 (-m32, 32bit)  testsuite:

New failures:
FAIL: gcc.target/i386/pr71245-1.c scan-assembler-not (fistp|fild)
FAIL: gcc.target/i386/pr71245-2.c scan-assembler-not movlps

Uros.

Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

[Alexander Monakov]

On Mon, 4 Sep 2017, Uros Bizjak wrote:
> introduced a couple of regressions on x86 (-m32, 32bit)  testsuite:
> 
> New failures:
> FAIL: gcc.target/i386/pr71245-1.c scan-assembler-not (fistp|fild)
> FAIL: gcc.target/i386/pr71245-2.c scan-assembler-not movlps

Sorry.  I suggest that the tests be XFAIL'ed, the peepholes introduced in the
fix for PR 71245 removed, and the PR reopened (it's a missed-optimization PR).
I can do all of the above if you agree.

I think RTL peepholes are a poor way of fixing the original problem, which
actually exists on all targets with separate int/fp registers.  For instance,
trunk (without my patch) still gets a far simpler testcase wrong (-O2, 64-bit):

float f(_Atomic float *p)
{
  return *p;
}

f:
        movl    (%rdi), %eax
        movl    %eax, -4(%rsp)
        movss   -4(%rsp), %xmm0
        ret

I believe adding more peepholes to handle this, independently in each affected
backend, is hardly appropriate.

The issue is caused by translation of floating-point atomic loads/stores to a
sequence of integer atomic and a VIEW_CONVERT_EXPR on GIMPLE, which in turn
causes the load to be emitted (wrongly) in an integer mode on RTL.  I don't
know if GIMPLE atomic builtins can work with floating operands (which IMHO
would be a preferable way), but if not, then special-casing a sequence of
integer atomic mem op and a VCE at expand time might be a cleaner way of
achieving the desired optimization.

Thanks.
Alexander

Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

[Uros Bizjak]

On Tue, Sep 5, 2017 at 12:28 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
> On Mon, 4 Sep 2017, Uros Bizjak wrote:
>> introduced a couple of regressions on x86 (-m32, 32bit)  testsuite:
>>
>> New failures:
>> FAIL: gcc.target/i386/pr71245-1.c scan-assembler-not (fistp|fild)
>> FAIL: gcc.target/i386/pr71245-2.c scan-assembler-not movlps
>
> Sorry.  I suggest that the tests be XFAIL'ed, the peepholes introduced in the
> fix for PR 71245 removed, and the PR reopened (it's a missed-optimization PR).
> I can do all of the above if you agree.

No, I have a solution for the regression. A prerequisite is patch at [1].

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

Uros.

Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

[Uros Bizjak]

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

On Tue, Sep 5, 2017 at 12:28 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
> On Mon, 4 Sep 2017, Uros Bizjak wrote:
>> introduced a couple of regressions on x86 (-m32, 32bit)  testsuite:
>>
>> New failures:
>> FAIL: gcc.target/i386/pr71245-1.c scan-assembler-not (fistp|fild)
>> FAIL: gcc.target/i386/pr71245-2.c scan-assembler-not movlps
>
> Sorry.  I suggest that the tests be XFAIL'ed, the peepholes introduced in the
> fix for PR 71245 removed, and the PR reopened (it's a missed-optimization PR).
> I can do all of the above if you agree.
>
> I think RTL peepholes are a poor way of fixing the original problem, which
> actually exists on all targets with separate int/fp registers.  For instance,
> trunk (without my patch) still gets a far simpler testcase wrong (-O2, 64-bit):

Please note that 32bit x86 implements atomic DImode access with
fild/fistp combination, so the mentioned peephole avoids quite costly
instruction sequence.

For reference, attached patch implements additional peephole2 patterns
that also handle sequences with blockages.

Uros.

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

diff --git a/gcc/config/i386/sync.md b/gcc/config/i386/sync.md
index 29b82f86d43a..eceaa73a6799 100644
--- a/gcc/config/i386/sync.md
+++ b/gcc/config/i386/sync.md
@@ -219,29 +219,71 @@
    (set (match_operand:DI 2 "memory_operand")
 	(unspec:DI [(match_dup 0)]
 		   UNSPEC_FIST_ATOMIC))
-   (set (match_operand:DF 3 "fp_register_operand")
+   (set (match_operand:DF 3 "any_fp_register_operand")
 	(match_operand:DF 4 "memory_operand"))]
   "!TARGET_64BIT
    && peep2_reg_dead_p (2, operands[0])
-   && rtx_equal_p (operands[4], adjust_address_nv (operands[2], DFmode, 0))"
+   && rtx_equal_p (XEXP (operands[4], 0), XEXP (operands[2], 0))"
   [(set (match_dup 3) (match_dup 5))]
   "operands[5] = gen_lowpart (DFmode, operands[1]);")
 
 (define_peephole2
+  [(set (match_operand:DF 0 "fp_register_operand")
+	(unspec:DF [(match_operand:DI 1 "memory_operand")]
+		   UNSPEC_FILD_ATOMIC))
+   (set (match_operand:DI 2 "memory_operand")
+	(unspec:DI [(match_dup 0)]
+		   UNSPEC_FIST_ATOMIC))
+   (set (mem:BLK (scratch:SI))
+	(unspec:BLK [(mem:BLK (scratch:SI))] UNSPEC_MEMORY_BLOCKAGE))
+   (set (match_operand:DF 3 "any_fp_register_operand")
+	(match_operand:DF 4 "memory_operand"))]
+  "!TARGET_64BIT
+   && peep2_reg_dead_p (2, operands[0])
+   && rtx_equal_p (XEXP (operands[4], 0), XEXP (operands[2], 0))"
+  [(const_int 0)]
+{
+  emit_move_insn (operands[3], gen_lowpart (DFmode, operands[1]));
+  emit_insn (gen_memory_blockage ());
+  DONE;
+})
+
+(define_peephole2
   [(set (match_operand:DF 0 "sse_reg_operand")
 	(unspec:DF [(match_operand:DI 1 "memory_operand")]
 		   UNSPEC_LDX_ATOMIC))
    (set (match_operand:DI 2 "memory_operand")
 	(unspec:DI [(match_dup 0)]
 		   UNSPEC_STX_ATOMIC))
-   (set (match_operand:DF 3 "fp_register_operand")
+   (set (match_operand:DF 3 "any_fp_register_operand")
 	(match_operand:DF 4 "memory_operand"))]
   "!TARGET_64BIT
    && peep2_reg_dead_p (2, operands[0])
-   && rtx_equal_p (operands[4], adjust_address_nv (operands[2], DFmode, 0))"
+   && rtx_equal_p (XEXP (operands[4], 0), XEXP (operands[2], 0))"
   [(set (match_dup 3) (match_dup 5))]
   "operands[5] = gen_lowpart (DFmode, operands[1]);")
 
+(define_peephole2
+  [(set (match_operand:DF 0 "sse_reg_operand")
+	(unspec:DF [(match_operand:DI 1 "memory_operand")]
+		   UNSPEC_LDX_ATOMIC))
+   (set (match_operand:DI 2 "memory_operand")
+	(unspec:DI [(match_dup 0)]
+		   UNSPEC_STX_ATOMIC))
+   (set (mem:BLK (scratch:SI))
+	(unspec:BLK [(mem:BLK (scratch:SI))] UNSPEC_MEMORY_BLOCKAGE))
+   (set (match_operand:DF 3 "any_fp_register_operand")
+	(match_operand:DF 4 "memory_operand"))]
+  "!TARGET_64BIT
+   && peep2_reg_dead_p (2, operands[0])
+   && rtx_equal_p (XEXP (operands[4], 0), XEXP (operands[2], 0))"
+  [(const_int 0)]
+{
+  emit_move_insn (operands[3], gen_lowpart (DFmode, operands[1]));
+  emit_insn (gen_memory_blockage ());
+  DONE;
+})
+
 (define_expand "atomic_store<mode>"
   [(set (match_operand:ATOMIC 0 "memory_operand")
 	(unspec:ATOMIC [(match_operand:ATOMIC 1 "nonimmediate_operand")
@@ -331,7 +373,7 @@
 
 (define_peephole2
   [(set (match_operand:DF 0 "memory_operand")
-	(match_operand:DF 1 "fp_register_operand"))
+	(match_operand:DF 1 "any_fp_register_operand"))
    (set (match_operand:DF 2 "fp_register_operand")
 	(unspec:DF [(match_operand:DI 3 "memory_operand")]
 		   UNSPEC_FILD_ATOMIC))
@@ -340,13 +382,34 @@
 		   UNSPEC_FIST_ATOMIC))]
   "!TARGET_64BIT
    && peep2_reg_dead_p (3, operands[2])
-   && rtx_equal_p (operands[0], adjust_address_nv (operands[3], DFmode, 0))"
+   && rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
   [(set (match_dup 5) (match_dup 1))]
   "operands[5] = gen_lowpart (DFmode, operands[4]);")
 
 (define_peephole2
   [(set (match_operand:DF 0 "memory_operand")
-	(match_operand:DF 1 "fp_register_operand"))
+	(match_operand:DF 1 "any_fp_register_operand"))
+   (set (mem:BLK (scratch:SI))
+	(unspec:BLK [(mem:BLK (scratch:SI))] UNSPEC_MEMORY_BLOCKAGE))
+   (set (match_operand:DF 2 "fp_register_operand")
+	(unspec:DF [(match_operand:DI 3 "memory_operand")]
+		   UNSPEC_FILD_ATOMIC))
+   (set (match_operand:DI 4 "memory_operand")
+	(unspec:DI [(match_dup 2)]
+		   UNSPEC_FIST_ATOMIC))]
+  "!TARGET_64BIT
+   && peep2_reg_dead_p (4, operands[2])
+   && rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
+  [(const_int 0)]
+{
+  emit_insn (gen_memory_blockage ());
+  emit_move_insn (gen_lowpart (DFmode, operands[4]), operands[1]);
+  DONE;
+})
+
+(define_peephole2
+  [(set (match_operand:DF 0 "memory_operand")
+	(match_operand:DF 1 "any_fp_register_operand"))
    (set (match_operand:DF 2 "sse_reg_operand")
 	(unspec:DF [(match_operand:DI 3 "memory_operand")]
 		   UNSPEC_LDX_ATOMIC))
@@ -355,10 +418,31 @@
 		   UNSPEC_STX_ATOMIC))]
   "!TARGET_64BIT
    && peep2_reg_dead_p (3, operands[2])
-   && rtx_equal_p (operands[0], adjust_address_nv (operands[3], DFmode, 0))"
+   && rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
   [(set (match_dup 5) (match_dup 1))]
   "operands[5] = gen_lowpart (DFmode, operands[4]);")
 
+(define_peephole2
+  [(set (match_operand:DF 0 "memory_operand")
+	(match_operand:DF 1 "any_fp_register_operand"))
+   (set (mem:BLK (scratch:SI))
+	(unspec:BLK [(mem:BLK (scratch:SI))] UNSPEC_MEMORY_BLOCKAGE))
+   (set (match_operand:DF 2 "sse_reg_operand")
+	(unspec:DF [(match_operand:DI 3 "memory_operand")]
+		   UNSPEC_LDX_ATOMIC))
+   (set (match_operand:DI 4 "memory_operand")
+	(unspec:DI [(match_dup 2)]
+		   UNSPEC_STX_ATOMIC))]
+  "!TARGET_64BIT
+   && peep2_reg_dead_p (4, operands[2])
+   && rtx_equal_p (XEXP (operands[0], 0), XEXP (operands[3], 0))"
+  [(const_int 0)]
+{
+  emit_insn (gen_memory_blockage ());
+  emit_move_insn (gen_lowpart (DFmode, operands[4]), operands[1]);
+  DONE;
+})
+
 ;; ??? You'd think that we'd be able to perform this via FLOAT + FIX_TRUNC
 ;; operations.  But the fix_trunc patterns want way more setup than we want
 ;; to provide.  Note that the scratch is DFmode instead of XFmode in order

Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

[Christophe Lyon]

Hi,

On 5 September 2017 at 13:40, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Tue, Sep 5, 2017 at 12:28 PM, Alexander Monakov <amonakov@ispras.ru> wrote:
>> On Mon, 4 Sep 2017, Uros Bizjak wrote:
>>> introduced a couple of regressions on x86 (-m32, 32bit)  testsuite:
>>>
>>> New failures:
>>> FAIL: gcc.target/i386/pr71245-1.c scan-assembler-not (fistp|fild)
>>> FAIL: gcc.target/i386/pr71245-2.c scan-assembler-not movlps
>>
>> Sorry.  I suggest that the tests be XFAIL'ed, the peepholes introduced in the
>> fix for PR 71245 removed, and the PR reopened (it's a missed-optimization PR).
>> I can do all of the above if you agree.
>>
>> I think RTL peepholes are a poor way of fixing the original problem, which
>> actually exists on all targets with separate int/fp registers.  For instance,
>> trunk (without my patch) still gets a far simpler testcase wrong (-O2, 64-bit):
>
> Please note that 32bit x86 implements atomic DImode access with
> fild/fistp combination, so the mentioned peephole avoids quite costly
> instruction sequence.
>
> For reference, attached patch implements additional peephole2 patterns
> that also handle sequences with blockages.
>
> Uros.

On arm, we also have a similar regression:
FAIL: gcc.target/arm/stl-cond.c scan-assembler stlne

Before the patch, we generated:
        ldr     r3, [r0]
        cmp     r3, #0
        addne   r3, r0, #4
        movne   r2, #0
        stlne   r2, [r3]
        bx      lr
and now:
        ldr     r3, [r0]
        cmp     r3, #0
        bxeq    lr
        mov     r2, #0
        add     r3, r0, #4
        stl     r2, [r3]
        bx      lr

Christophe

Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

[Jeff Law]

On 09/01/2017 12:26 AM, Alexander Monakov wrote:
> On Thu, 31 Aug 2017, Jeff Law wrote:
>> This is OK.
>>
>> What's the point of the delete_insns_since calls in patch #3?
> 
> Deleting the first barrier when maybe_expand_insn failed.
> Other functions in the file use a similar approach.
Thanks for clarifying.  OK for the trunk.  Sorry about the wait.

Jeff

Re: [PING][PATCH 2/3] retire mem_signal_fence pattern

[Alexander Monakov]

On Thu, 31 Aug 2017, Jeff Law wrote:
> This is OK.
> 
> What's the point of the delete_insns_since calls in patch #3?

Deleting the first barrier when maybe_expand_insn failed.
Other functions in the file use a similar approach.

Thanks.
Alexander

[PING][PATCH 2/3] retire mem_signal_fence pattern

[Alexander Monakov]

Ping (for this and patch 3/3 in the thread).

On Wed, 2 Aug 2017, Alexander Monakov wrote:

> Similar to mem_thread_fence issue from the patch 1/3, RTL representation of
> __atomic_signal_fence must be a compiler barrier.  We have just one backend
> offering this pattern, and it does not place a compiler barrier.
> 
> It does not appear useful to expand signal_fence to some kind of hardware
> instruction, its documentation is wrong/outdated, and we are using it
> incorrectly anyway.  So just remove the whole thing and simply emit a compiler
> memory barrier in the optabs.c handler.
> 
> 	* config/s390/s390.md (mem_signal_fence): Remove.
>         * doc/md.texi (mem_signal_fence): Remove.
>         * optabs.c (expand_mem_signal_fence): Remove uses of mem_signal_fence.
>         Update comments.
>         * target-insns.def (mem_signal_fence): Remove.
> 
> ---
>  gcc/config/s390/s390.md |  9 ---------
>  gcc/doc/md.texi         | 13 -------------
>  gcc/optabs.c            | 17 +++++------------
>  gcc/target-insns.def    |  1 -
>  4 files changed, 5 insertions(+), 35 deletions(-)
> 
> diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
> index d1ac0b8395d..1d63523d3b0 100644
> --- a/gcc/config/s390/s390.md
> +++ b/gcc/config/s390/s390.md
> @@ -10084,15 +10084,6 @@ (define_insn "*basr_tls"
>  ; memory barrier patterns.
>  ;
>  
> -(define_expand "mem_signal_fence"
> -  [(match_operand:SI 0 "const_int_operand")]		;; model
> -  ""
> -{
> -  /* The s390 memory model is strong enough not to require any
> -     barrier in order to synchronize a thread with itself.  */
> -  DONE;
> -})
> -
>  (define_expand "mem_thread_fence"
>    [(match_operand:SI 0 "const_int_operand")]		;; model
>    ""
> diff --git a/gcc/doc/md.texi b/gcc/doc/md.texi
> index 0be161a08dc..73d0e7d83c0 100644
> --- a/gcc/doc/md.texi
> +++ b/gcc/doc/md.texi
> @@ -7058,19 +7058,6 @@ If this pattern is not specified, all memory models except
>  @code{__ATOMIC_RELAXED} will result in issuing a @code{sync_synchronize}
>  barrier pattern.
>  
> -@cindex @code{mem_signal_fence@var{mode}} instruction pattern
> -@item @samp{mem_signal_fence@var{mode}}
> -This pattern emits code required to implement a signal fence with
> -memory model semantics.  Operand 0 is the memory model to be used.
> -
> -This pattern should impact the compiler optimizers the same way that
> -mem_signal_fence does, but it does not need to issue any barrier
> -instructions.
> -
> -If this pattern is not specified, all memory models except
> -@code{__ATOMIC_RELAXED} will result in issuing a @code{sync_synchronize}
> -barrier pattern.
> -
>  @cindex @code{get_thread_pointer@var{mode}} instruction pattern
>  @cindex @code{set_thread_pointer@var{mode}} instruction pattern
>  @item @samp{get_thread_pointer@var{mode}}
> diff --git a/gcc/optabs.c b/gcc/optabs.c
> index d568ca376fe..6884fd4b8aa 100644
> --- a/gcc/optabs.c
> +++ b/gcc/optabs.c
> @@ -6315,22 +6315,15 @@ expand_mem_thread_fence (enum memmodel model)
>      }
>  }
>  
> -/* This routine will either emit the mem_signal_fence pattern or issue a 
> -   sync_synchronize to generate a fence for memory model MEMMODEL.  */
> +/* Emit a signal fence with given memory model.  */
>  
>  void
>  expand_mem_signal_fence (enum memmodel model)
>  {
> -  if (targetm.have_mem_signal_fence ())
> -    emit_insn (targetm.gen_mem_signal_fence (GEN_INT (model)));
> -  else if (!is_mm_relaxed (model))
> -    {
> -      /* By default targets are coherent between a thread and the signal
> -	 handler running on the same thread.  Thus this really becomes a
> -	 compiler barrier, in that stores must not be sunk past
> -	 (or raised above) a given point.  */
> -      expand_asm_memory_barrier ();
> -    }
> +  /* 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 ();
>  }
>  
>  /* This function expands the atomic load operation:
> diff --git a/gcc/target-insns.def b/gcc/target-insns.def
> index fb92f72ac29..4669439c7e1 100644
> --- a/gcc/target-insns.def
> +++ b/gcc/target-insns.def
> @@ -58,7 +58,6 @@ DEF_TARGET_INSN (indirect_jump, (rtx x0))
>  DEF_TARGET_INSN (insv, (rtx x0, rtx x1, rtx x2, rtx x3))
>  DEF_TARGET_INSN (jump, (rtx x0))
>  DEF_TARGET_INSN (load_multiple, (rtx x0, rtx x1, rtx x2))
> -DEF_TARGET_INSN (mem_signal_fence, (rtx x0))
>  DEF_TARGET_INSN (mem_thread_fence, (rtx x0))
>  DEF_TARGET_INSN (memory_barrier, (void))
>  DEF_TARGET_INSN (movstr, (rtx x0, rtx x1, rtx x2))
>