[PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Uros Bizjak]

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

Hello!

Attached patch extends TARGET_READ_MODIFY{,_WRITE} peepholes to handle
all integer modes, while also taking care not to introduce additional
QImode register stalls.

While looking at the insn enable condition, I noticed that we don't
use "probe_stack" pattern any more, as the stack check loop is now
implemented in a different way.

2016-04-28  Uros Bizjak  <ubizjak@gmail.com>

    * config/i386/i386.md (peephole2s for operations with memory inputs):
    Use SWI mode iterator.
    (peephole2s for operations with memory outputs): Ditto.
    Do not check for stack checking probe.

    (probe_stack): Remove expander.

Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Committed to mainline SVN.

Uros.

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

Index: config/i386/i386.md
===================================================================
--- config/i386/i386.md	(revision 235582)
+++ config/i386/i386.md	(working copy)
@@ -17552,20 +17552,6 @@
   DONE;
 })
 
-;; Use IOR for stack probes, this is shorter.
-(define_expand "probe_stack"
-  [(match_operand 0 "memory_operand")]
-  ""
-{
-  rtx (*gen_ior3) (rtx, rtx, rtx);
-
-  gen_ior3 = (GET_MODE (operands[0]) == DImode
-	      ? gen_iordi3 : gen_iorsi3);
-
-  emit_insn (gen_ior3 (operands[0], operands[0], const0_rtx));
-  DONE;
-})
-
 (define_insn "adjust_stack_and_probe<mode>"
   [(set (match_operand:P 0 "register_operand" "=r")
 	(unspec_volatile:P [(match_operand:P 1 "register_operand" "0")]
@@ -17894,11 +17880,11 @@
 
 ;; Don't do logical operations with memory inputs.
 (define_peephole2
-  [(match_scratch:SI 2 "r")
-   (parallel [(set (match_operand:SI 0 "register_operand")
-                   (match_operator:SI 3 "arith_or_logical_operator"
+  [(match_scratch:SWI 2 "<r>")
+   (parallel [(set (match_operand:SWI 0 "register_operand")
+		   (match_operator:SWI 3 "arith_or_logical_operator"
                      [(match_dup 0)
-                      (match_operand:SI 1 "memory_operand")]))
+		      (match_operand:SWI 1 "memory_operand")]))
               (clobber (reg:CC FLAGS_REG))])]
   "!(TARGET_READ_MODIFY || optimize_insn_for_size_p ())"
   [(set (match_dup 2) (match_dup 1))
@@ -17907,10 +17893,10 @@
               (clobber (reg:CC FLAGS_REG))])])
 
 (define_peephole2
-  [(match_scratch:SI 2 "r")
-   (parallel [(set (match_operand:SI 0 "register_operand")
-                   (match_operator:SI 3 "arith_or_logical_operator"
-                     [(match_operand:SI 1 "memory_operand")
+  [(match_scratch:SWI 2 "<r>")
+   (parallel [(set (match_operand:SWI 0 "register_operand")
+		   (match_operator:SWI 3 "arith_or_logical_operator"
+		     [(match_operand:SWI 1 "memory_operand")
                       (match_dup 0)]))
               (clobber (reg:CC FLAGS_REG))])]
   "!(TARGET_READ_MODIFY || optimize_insn_for_size_p ())"
@@ -17962,15 +17948,13 @@
 ; the same decoder scheduling characteristics as the original.
 
 (define_peephole2
-  [(match_scratch:SI 2 "r")
-   (parallel [(set (match_operand:SI 0 "memory_operand")
-                   (match_operator:SI 3 "arith_or_logical_operator"
+  [(match_scratch:SWI 2 "<r>")
+   (parallel [(set (match_operand:SWI 0 "memory_operand")
+		   (match_operator:SWI 3 "arith_or_logical_operator"
                      [(match_dup 0)
-                      (match_operand:SI 1 "nonmemory_operand")]))
+		      (match_operand:SWI 1 "<nonmemory_operand>")]))
               (clobber (reg:CC FLAGS_REG))])]
-  "!(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
-   /* Do not split stack checking probes.  */
-   && GET_CODE (operands[3]) != IOR && operands[1] != const0_rtx"
+  "!(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())"
   [(set (match_dup 2) (match_dup 0))
    (parallel [(set (match_dup 2)
                    (match_op_dup 3 [(match_dup 2) (match_dup 1)]))
@@ -17978,15 +17962,13 @@
    (set (match_dup 0) (match_dup 2))])
 
 (define_peephole2
-  [(match_scratch:SI 2 "r")
-   (parallel [(set (match_operand:SI 0 "memory_operand")
-                   (match_operator:SI 3 "arith_or_logical_operator"
-                     [(match_operand:SI 1 "nonmemory_operand")
+  [(match_scratch:SWI 2 "<r>")
+   (parallel [(set (match_operand:SWI 0 "memory_operand")
+		   (match_operator:SWI 3 "arith_or_logical_operator"
+		     [(match_operand:SWI 1 "<nonmemory_operand>")
                       (match_dup 0)]))
               (clobber (reg:CC FLAGS_REG))])]
-  "!(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())
-   /* Do not split stack checking probes.  */
-   && GET_CODE (operands[3]) != IOR && operands[1] != const0_rtx"
+  "!(TARGET_READ_MODIFY_WRITE || optimize_insn_for_size_p ())"
   [(set (match_dup 2) (match_dup 0))
    (parallel [(set (match_dup 2)
                    (match_op_dup 3 [(match_dup 1) (match_dup 2)]))

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Eric Botcazou]

> While looking at the insn enable condition, I noticed that we don't
> use "probe_stack" pattern any more, as the stack check loop is now
> implemented in a different way.

Yes, we do, probe_stack is a standard pattern called by the middle-end.

> 2016-04-28  Uros Bizjak  <ubizjak@gmail.com>
> 
>     * config/i386/i386.md (peephole2s for operations with memory inputs):
>     Use SWI mode iterator.
>     (peephole2s for operations with memory outputs): Ditto.
>     Do not check for stack checking probe.
> 
>     (probe_stack): Remove expander.
> 
> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

How did you test it exactly?

                === acats tests ===
FAIL:   c52103x
FAIL:   c52104x

-- 
Eric Botcazou

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Uros Bizjak]

On Fri, Apr 29, 2016 at 9:47 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> While looking at the insn enable condition, I noticed that we don't
>> use "probe_stack" pattern any more, as the stack check loop is now
>> implemented in a different way.
>
> Yes, we do, probe_stack is a standard pattern called by the middle-end.
>
>> 2016-04-28  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     * config/i386/i386.md (peephole2s for operations with memory inputs):
>>     Use SWI mode iterator.
>>     (peephole2s for operations with memory outputs): Ditto.
>>     Do not check for stack checking probe.
>>
>>     (probe_stack): Remove expander.
>>
>> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> How did you test it exactly?
>
>                 === acats tests ===
> FAIL:   c52103x
> FAIL:   c52104x

Apparently without ada...

We can put it back, but prehaps implemented as unspec, so it won't
interfere with peepholes?

Uros.

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Eric Botcazou]

> We can put it back, but prehaps implemented as unspec, so it won't
> interfere with peepholes?

No strong opinion, as long as the final assembly is the same as before.

-- 
Eric Botcazou

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Uros Bizjak]

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

On Fri, Apr 29, 2016 at 9:58 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> We can put it back, but prehaps implemented as unspec, so it won't
>> interfere with peepholes?
>
> No strong opinion, as long as the final assembly is the same as before.

I'm testing the attached patch. Does it fix your ada failures?

Uros.

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

Index: i386.md
===================================================================
--- i386.md	(revision 235620)
+++ i386.md	(working copy)
@@ -88,6 +88,7 @@
   UNSPEC_SET_GOT_OFFSET
   UNSPEC_MEMORY_BLOCKAGE
   UNSPEC_STACK_CHECK
+  UNSPEC_PROBE_STACK
 
   ;; TLS support
   UNSPEC_TP
@@ -17552,6 +17553,23 @@
   DONE;
 })
 
+(define_expand "probe_stack"
+  [(parallel
+     [(set (match_operand 0 "memory_operand")
+	   (unspec [(const_int 0)] UNSPEC_PROBE_STACK))
+      (clobber (reg:CC FLAGS_REG))])])
+
+;; Use OR for stack probes, this is shorter.
+(define_insn "*probe_stack_<mode>"
+  [(set (match_operand:W 0 "memory_operand" "=m")
+	(unspec:W [(const_int 0)] UNSPEC_PROBE_STACK))
+   (clobber (reg:CC FLAGS_REG))]
+  ""
+  "or{<imodesuffix>}\t{$0, %0|%0, 0}"
+  [(set_attr "type" "alu1")
+   (set_attr "mode" "<MODE>")
+   (set_attr "length_immediate" "1")])
+  
 (define_insn "adjust_stack_and_probe<mode>"
   [(set (match_operand:P 0 "register_operand" "=r")
 	(unspec_volatile:P [(match_operand:P 1 "register_operand" "0")]

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Eric Botcazou]

> I'm testing the attached patch. Does it fix your ada failures?

No, it totally breaks stack checking. :-(

                === acats tests ===
 FAIL:  c52103x
 FAIL:  c52104x
+FAIL:  c52104y
+FAIL:  cb1010a
+FAIL:  cb1010c
+FAIL:  cb1010d
 
                === acats Summary ===
-# of expected passes           2318
-# of unexpected failures       2
+# of expected passes           2314
+# of unexpected failures       6
 Native configuration is x86_64-suse-linux-gnu
 
                === gcc tests ===
@@ -133,11 +137,24 @@
 
 
 Running target unix
+FAIL: gnat.dg/opt49.adb 3 blank line(s) in output
+FAIL: gnat.dg/opt49.adb (test for excess errors)
+UNRESOLVED: gnat.dg/opt49.adb compilation failed to produce executable
+FAIL: gnat.dg/stack_check1.adb 3 blank line(s) in output
+FAIL: gnat.dg/stack_check1.adb (test for excess errors)
+UNRESOLVED: gnat.dg/stack_check1.adb compilation failed to produce executable
+FAIL: gnat.dg/stack_check2.adb 3 blank line(s) in output
+FAIL: gnat.dg/stack_check2.adb (test for excess errors)
+UNRESOLVED: gnat.dg/stack_check2.adb compilation failed to produce executable
+FAIL: gnat.dg/stack_check3.adb 3 blank line(s) in output
+FAIL: gnat.dg/stack_check3.adb (test for excess errors)


/home/eric/svn/gcc/gcc/testsuite/gnat.dg/opt49.adb:31:4: error: unrecognizable 
insn:
(insn 33 32 34 8 (parallel [
            (set (mem/v:DI (reg/f:DI 7 sp) [0  S8 A8])
                (unspec [
                        (const_int 0 [0])
                    ] UNSPEC_PROBE_STACK))
            (clobber (reg:CC 17 flags))
        ]) /home/eric/svn/gcc/gcc/testsuite/gnat.dg/opt49.adb:17 -1
     (nil))
+===========================GNAT BUG DETECTED==============================+
| 7.0.0 20160429 (experimental) [trunk revision 235619] (x86_64-suse-linux) 
GCC error:|
| in extract_insn, at recog.c:2287                                         |
| Error detected around 
/home/eric/svn/gcc/gcc/testsuite/gnat.dg/opt49.adb:31:4|

-- 
Eric Botcazou

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Uros Bizjak]

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

On Fri, Apr 29, 2016 at 12:17 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I'm testing the attached patch. Does it fix your ada failures?
>
> No, it totally breaks stack checking. :-(

Eh, I was trying to be too clever.

Attached patch was actually tested on a couple of cases. It generates
the same assembly as before.

Uros.

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

Index: i386.md
===================================================================
--- i386.md	(revision 235620)
+++ i386.md	(working copy)
@@ -88,6 +88,7 @@
   UNSPEC_SET_GOT_OFFSET
   UNSPEC_MEMORY_BLOCKAGE
   UNSPEC_STACK_CHECK
+  UNSPEC_PROBE_STACK
 
   ;; TLS support
   UNSPEC_TP
@@ -17552,6 +17553,29 @@
   DONE;
 })
 
+(define_expand "probe_stack"
+  [(match_operand 0 "memory_operand")]
+  ""
+{
+  rtx (*insn) (rtx)
+    = (GET_MODE (operands[0]) == DImode
+       ? gen_probe_stack_di : gen_probe_stack_si);
+
+  emit_insn (insn (operands[0]));
+  DONE;
+})
+
+;; Use OR for stack probes, this is shorter.
+(define_insn "probe_stack_<mode>"
+  [(set (match_operand:W 0 "memory_operand" "=m")
+	(unspec:W [(const_int 0)] UNSPEC_PROBE_STACK))
+   (clobber (reg:CC FLAGS_REG))]
+  ""
+  "or{<imodesuffix>}\t{$0, %0|%0, 0}"
+  [(set_attr "type" "alu1")
+   (set_attr "mode" "<MODE>")
+   (set_attr "length_immediate" "1")])
+  
 (define_insn "adjust_stack_and_probe<mode>"
   [(set (match_operand:P 0 "register_operand" "=r")
 	(unspec_volatile:P [(match_operand:P 1 "register_operand" "0")]

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Eric Botcazou]

> Attached patch was actually tested on a couple of cases. It generates
> the same assembly as before.

Note that you could just remove the second ":W" in the define_insn pattern.

That's better, but not quite it because this segfaults at -O2:

#0  memory_operand (op=0xabababababababab, mode=mode@entry=VOIDmode)
    at /home/eric/svn/gcc/gcc/recog.c:1360
#1  0x00000000014388b1 in get_attr_memory (insn=insn@entry=0x7ffff697b8c0)
    at /home/eric/svn/gcc/gcc/config/i386/i386.md:2120
#2  0x0000000001636fb8 in insn_default_latency_generic (insn=0x7ffff697b8c0)
    at /home/eric/svn/gcc/gcc/config/i386/i386.md:27394
#3  0x00000000017f9695 in insn_cost (insn=0x7ffff697b8c0)
    at /home/eric/svn/gcc/gcc/haifa-sched.c:1415
#4  0x00000000017feb75 in dep_cost_1 (link=link@entry=0x2e962e8, 
dw=dw@entry=0)
    at /home/eric/svn/gcc/gcc/haifa-sched.c:1468
#5  0x0000000001800d7a in dep_cost (link=0x2e962e8)
    at /home/eric/svn/gcc/gcc/haifa-sched.c:1523
#6  priority (insn=0x7ffff697b8c0) at /home/eric/svn/gcc/gcc/haifa-
sched.c:1674
#7  0x0000000001800e6f in set_priorities (head=<optimized out>, 
    tail=<optimized out>) at /home/eric/svn/gcc/gcc/haifa-sched.c:7209
#8  0x0000000000f689e3 in compute_priorities ()
    at /home/eric/svn/gcc/gcc/sched-rgn.c:3022
#9  0x0000000000f6bc46 in schedule_region (rgn=0)
    at /home/eric/svn/gcc/gcc/sched-rgn.c:3115
#10 schedule_insns () at /home/eric/svn/gcc/gcc/sched-rgn.c:3513
#11 0x0000000000f6c4de in schedule_insns ()

(gdb) frame 1
#1  0x00000000014388b1 in get_attr_memory (insn=insn@entry=0x7ffff697b8c0)
    at /home/eric/svn/gcc/gcc/config/i386/i386.md:2120
2120                   (match_test "TARGET_AVX")
(gdb) p debug_rtx(insn)
(insn 779 927 928 2 (parallel [
            (set (mem/v:DI (reg/f:DI 7 sp) [0  S8 A8])
                (unspec:DI [
                        (const_int 0 [0])
                    ] UNSPEC_PROBE_STACK))
            (clobber (reg:CC 17 flags))
        ]) c52104y.adb:57 1005 {probe_stack_di}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (nil)))
$1 = void

-- 
Eric Botcazou

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Uros Bizjak]

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

On Fri, Apr 29, 2016 at 1:23 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Attached patch was actually tested on a couple of cases. It generates
>> the same assembly as before.
>
> Note that you could just remove the second ":W" in the define_insn pattern.

Eh, then the build yawns about the missing mode of the input operand.

> That's better, but not quite it because this segfaults at -O2:

This is getting a bit frustrating, but attached patch should solve
this failure. Again lightly tested, regtest in progress.

Uros.

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

Index: i386.md
===================================================================
--- i386.md	(revision 235620)
+++ i386.md	(working copy)
@@ -88,6 +88,7 @@
   UNSPEC_SET_GOT_OFFSET
   UNSPEC_MEMORY_BLOCKAGE
   UNSPEC_STACK_CHECK
+  UNSPEC_PROBE_STACK
 
   ;; TLS support
   UNSPEC_TP
@@ -17552,6 +17553,30 @@
   DONE;
 })
 
+(define_expand "probe_stack"
+  [(match_operand 0 "memory_operand")]
+  ""
+{
+  rtx (*insn) (rtx, rtx)
+    = (GET_MODE (operands[0]) == DImode
+       ? gen_probe_stack_di : gen_probe_stack_si);
+
+  emit_insn (insn (operands[0], const0_rtx));
+  DONE;
+})
+
+;; Use OR for stack probes, this is shorter.
+(define_insn "probe_stack_<mode>"
+  [(set (match_operand:W 0 "memory_operand" "=m")
+	(unspec:W [(match_operand:W 1 "const0_operand")]
+		  UNSPEC_PROBE_STACK))
+   (clobber (reg:CC FLAGS_REG))]
+  ""
+  "or{<imodesuffix>}\t{%1, %0|%0, %1}"
+  [(set_attr "type" "alu1")
+   (set_attr "mode" "<MODE>")
+   (set_attr "length_immediate" "1")])
+  
 (define_insn "adjust_stack_and_probe<mode>"
   [(set (match_operand:P 0 "register_operand" "=r")
 	(unspec_volatile:P [(match_operand:P 1 "register_operand" "0")]

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Eric Botcazou]

> Eh, then the build yawns about the missing mode of the input operand.

Every good back-end has at least an example of this. ;-)

> This is getting a bit frustrating, but attached patch should solve
> this failure. Again lightly tested, regtest in progress.

Everything is back to normal with this one, thanks!

-- 
Eric Botcazou

Re: [PATCH, i386]: Extend TARGET_READ_MODIFY{,_WRITE} peepholes to all integer modes

[Uros Bizjak]

On Fri, Apr 29, 2016 at 4:30 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Eh, then the build yawns about the missing mode of the input operand.
>
> Every good back-end has at least an example of this. ;-)
>
>> This is getting a bit frustrating, but attached patch should solve
>> this failure. Again lightly tested, regtest in progress.
>
> Everything is back to normal with this one, thanks!

Thanks for testing, committed to mainline with following ChangeLog:

2016-04-29  Uros Bizjak  <ubizjak@gmail.com>

    * config/i386/i386.md (unspec): Add UNSPEC_PROBE_STACK.
    (probe_stack): New expander.
    (probe_stack_<mode>): New insn pattern.

Uros.