Re: S/390: Fix warnings in "*setmem_long..." patterns.

[Dominik Vogt <vogt@linux.vnet.ibm.com>]

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

On Mon, Nov 30, 2015 at 06:11:33PM +0100, Ulrich Weigand wrote:
> On 11/30/2015 04:11 PM, Dominik Vogt wrote:
> > The attached patch fixes some warnings generated by the setmem...
> > patterns in s390.md during build and add test cases for the
> > patterns.  The patch is to be added on to p of the movstr patch:
> > https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03485.html
> > 
> > The test cases validate that the patterns are actually used, but
> > at the moment the setmem_long_and pattern is never actually used
> > and thus the test case would fail.  So I've split the patch in two
> > (both attached to this message) to activate this part of the test
> > once we've fixed that.
> > 
> > The patch has passed the SPEC2006 testsuite without any measurable
> > changes in performance.
> 
> What would you think about something like the following?
> 
> (define_insn "*setmem_long"
>   [(clobber (match_operand:<DBL> 0 "register_operand" "=d"))
>    (set (mem:BLK (subreg:P (match_operand:<DBL> 3 "register_operand" "0") 0))
>         (unspec:BLK [(match_operand:P 2 "shift_count_or_setmem_operand" "Y")
>                      (subreg:P (match_dup 3) 1)] UNSPEC_REPLICATE_BYTE))
>    (use (match_operand:<DBL> 1 "register_operand" "d"))
>    (clobber (reg:CC CC_REGNUM))]

New patch attached (patch 1.5 and ChangeLog are the same).  I've
swapped the operands 1 and 3 so that the numbering is the same as
before.  I think there are still a couple of problems with the
patched code:

1.

The new pattern has "(use (match_operand 3))" where the old one
just had match_dup (which did not express that a register pair was
required).  The expander function now requires a fourth, unused
argument that I don't know how to get rid of.

  emit_insn (gen_setmem_long_di (dst, convert_to_mode (Pmode, len, 1),
                                      val, NULL_RTX));
                                           ^^^^^^^^

2.

I think the pattern should express that the register pair with the
destination address and length gets clobbered by the mvcle
instruction, and I'm not sure whether it's necessary to tell Gcc
explicitly that the register pair with the source address and
legth gets zeroed.

> [ Not sure if we'd need an extra (use (match_dup 3)) any more. ]
> 
> B.t.w. this is certainly wrong and cannot be generated by common code:
>         (and:BLK (unspec:BLK
> 	      [(match_operand:P 2 "shift_count_or_setmem_operand" "Y")]
> 	      UNSPEC_P_TO_BLK)
>  	     (match_operand 4 "const_int_operand"             "n"))
> (This explains why the pattern would never match.)

It never matched before this change either.

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany

[-- Attachment #2: 0001-0001b-v2-Changelog --]
[-- Type: text/plain, Size: 368 bytes --]

gcc/ChangeLog

	* config/s390/s390.c (s390_expand_setmem): Use new expanders.
	* config/s390/s390.md ("*setmem_long")
	("*setmem_long_and", "*setmem_long_31z"): Fix warnings.
	("setmem_long_<P:mode>"): New expanders.
	("setmem_long"): Removed.

gcc/testsuite/ChangeLog

	* gcc.target/s390/md/setmem_long-1.c: New test.
	* gcc.target/s390/md/setmem_long-2.c: New test.

[-- Attachment #3: 0001-v2-S-390-Fix-warnings-in-setmem_long.-patterns.patch --]
[-- Type: text/x-diff, Size: 6714 bytes --]

From 0e1bc4be3466b0f07b1d5c1334e3717802a7db82 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Wed, 4 Nov 2015 03:16:24 +0100
Subject: [PATCH 1/1.5] S/390: Fix warnings in "*setmem_long..." patterns.

---
 gcc/config/s390/s390.c                           |  7 +++-
 gcc/config/s390/s390.md                          | 51 ++++++++++++++----------
 gcc/testsuite/gcc.target/s390/md/setmem_long-1.c | 20 ++++++++++
 gcc/testsuite/gcc.target/s390/md/setmem_long-2.c | 20 ++++++++++
 4 files changed, 75 insertions(+), 23 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/s390/md/setmem_long-1.c
 create mode 100644 gcc/testsuite/gcc.target/s390/md/setmem_long-2.c

diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index 40ee2f7..df7af91 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -5178,7 +5178,12 @@ s390_expand_setmem (rtx dst, rtx len, rtx val)
   else if (TARGET_MVCLE)
     {
       val = force_not_mem (convert_modes (Pmode, QImode, val, 1));
-      emit_insn (gen_setmem_long (dst, convert_to_mode (Pmode, len, 1), val));
+      if (TARGET_64BIT)
+	emit_insn (gen_setmem_long_di (dst, convert_to_mode (Pmode, len, 1),
+				       val, NULL_RTX));
+      else
+	emit_insn (gen_setmem_long_si (dst, convert_to_mode (Pmode, len, 1),
+				       val, NULL_RTX));
     }
 
   else
diff --git a/gcc/config/s390/s390.md b/gcc/config/s390/s390.md
index 75e9af7..e093fd3 100644
--- a/gcc/config/s390/s390.md
+++ b/gcc/config/s390/s390.md
@@ -70,6 +70,9 @@
    ; Copy CC as is into the lower 2 bits of an integer register
    UNSPEC_CC_TO_INT
 
+   ; Convert Pmode to BLKmode
+   UNSPEC_REPLICATE_BYTE
+
    ; GOT/PLT and lt-relative accesses
    UNSPEC_LTREL_OFFSET
    UNSPEC_LTREL_BASE
@@ -3281,13 +3284,13 @@
 
 ; Initialize a block of arbitrary length with (operands[2] % 256).
 
-(define_expand "setmem_long"
+(define_expand "setmem_long_<P:mode>"
   [(parallel
-    [(clobber (match_dup 1))
-     (set (match_operand:BLK 0 "memory_operand" "")
-          (match_operand 2 "shift_count_or_setmem_operand" ""))
-     (use (match_operand 1 "general_operand" ""))
-     (use (match_dup 3))
+    [(clobber (match_operand:<DBL> 0 "register_operand" "=d"))
+     (set (mem:BLK (subreg:P (match_operand:<DBL> 1 "register_operand" "0") 0))
+	  (unspec:BLK [(match_operand:P 2 "shift_count_or_setmem_operand" "Y")
+		      (subreg:P (match_dup 1) 8)] UNSPEC_REPLICATE_BYTE))
+     (use (match_operand:<DBL> 3 "register_operand" "d"))
      (clobber (reg:CC CC_REGNUM))])]
   ""
 {
@@ -3310,11 +3313,12 @@
 })
 
 (define_insn "*setmem_long"
-  [(clobber (match_operand:<DBL> 0 "register_operand" "=d"))
-   (set (mem:BLK (subreg:P (match_operand:<DBL> 3 "register_operand" "0") 0))
-        (match_operand 2 "shift_count_or_setmem_operand" "Y"))
-   (use (match_dup 3))
-   (use (match_operand:<DBL> 1 "register_operand" "d"))
+  [(clobber
+    (mem:BLK (subreg:P (match_operand:<DBL> 0 "register_operand" "=d") 0)))
+   (set (mem:BLK (subreg:P (match_operand:<DBL> 1 "register_operand" "0") 0))
+        (unspec:BLK [(match_operand:P 2 "shift_count_or_setmem_operand" "Y")
+                     (subreg:P (match_dup 1) 8)] UNSPEC_REPLICATE_BYTE))
+   (use (match_operand:<DBL> 3 "register_operand" "d"))
    (clobber (reg:CC CC_REGNUM))]
   "TARGET_64BIT || !TARGET_ZARCH"
   "mvcle\t%0,%1,%Y2\;jo\t.-4"
@@ -3322,12 +3326,14 @@
    (set_attr "type" "vs")])
 
 (define_insn "*setmem_long_and"
-  [(clobber (match_operand:<DBL> 0 "register_operand" "=d"))
-   (set (mem:BLK (subreg:P (match_operand:<DBL> 3 "register_operand" "0") 0))
-        (and (match_operand 2 "shift_count_or_setmem_operand" "Y")
-	     (match_operand 4 "const_int_operand"             "n")))
-   (use (match_dup 3))
-   (use (match_operand:<DBL> 1 "register_operand" "d"))
+  [(clobber
+    (mem:BLK (subreg:P (match_operand:<DBL> 0 "register_operand" "=d") 0)))
+   (set (mem:BLK (subreg:P (match_operand:<DBL> 1 "register_operand" "0") 0))
+        (unspec:BLK [(and:P
+		      (match_operand:P 2 "shift_count_or_setmem_operand" "Y")
+		      (match_operand:P 4 "const_int_operand"             "n"))
+		    (subreg:P (match_dup 1) 8)] UNSPEC_REPLICATE_BYTE))
+   (use (match_operand:<DBL> 3 "register_operand" "d"))
    (clobber (reg:CC CC_REGNUM))]
   "(TARGET_64BIT || !TARGET_ZARCH) &&
    (INTVAL (operands[4]) & 255) == 255"
@@ -3336,11 +3342,12 @@
    (set_attr "type" "vs")])
 
 (define_insn "*setmem_long_31z"
-  [(clobber (match_operand:TI 0 "register_operand" "=d"))
-   (set (mem:BLK (subreg:SI (match_operand:TI 3 "register_operand" "0") 4))
-        (match_operand 2 "shift_count_or_setmem_operand" "Y"))
-   (use (match_dup 3))
-   (use (match_operand:TI 1 "register_operand" "d"))
+  [(clobber
+    (mem:BLK (subreg:SI (match_operand:TI 0 "register_operand" "=d") 4)))
+   (set (mem:BLK (subreg:SI (match_operand:TI 1 "register_operand" "0") 0))
+        (unspec:BLK [(match_operand:P 2 "shift_count_or_setmem_operand" "Y")
+                     (subreg:P (match_dup 1) 8)] UNSPEC_REPLICATE_BYTE))
+   (use (match_operand:TI 3 "register_operand" "d"))
    (clobber (reg:CC CC_REGNUM))]
   "!TARGET_64BIT && TARGET_ZARCH"
   "mvcle\t%0,%1,%Y2\;jo\t.-4"
diff --git a/gcc/testsuite/gcc.target/s390/md/setmem_long-1.c b/gcc/testsuite/gcc.target/s390/md/setmem_long-1.c
new file mode 100644
index 0000000..9a926ce
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/md/setmem_long-1.c
@@ -0,0 +1,20 @@
+/* Machine description pattern tests.  */
+
+/* { dg-do compile { target { lp64 } } } */
+/* { dg-options "-mmvcle -dP" } */
+
+#include <string.h>
+
+void test(char *p, char c)
+{
+  __builtin_memset (p, c, 10000);
+}
+
+void test2(char *p, int c)
+{
+  __builtin_memset (p, (char)c, 10000);
+}
+
+/* Check that the right patterns are used.  */
+/* { dg-final { scan-assembler-times "c:10 .\*\{\\*setmem_long\}" 1 } } */
+/* { dg-final { scan-assembler-times "c:15 .\*\{\\*setmem_long\}" 1 } } */
diff --git a/gcc/testsuite/gcc.target/s390/md/setmem_long-2.c b/gcc/testsuite/gcc.target/s390/md/setmem_long-2.c
new file mode 100644
index 0000000..ce6aa42
--- /dev/null
+++ b/gcc/testsuite/gcc.target/s390/md/setmem_long-2.c
@@ -0,0 +1,20 @@
+/* Machine description pattern tests.  */
+
+/* { dg-do compile { target { ! lp64 } } } */
+/* { dg-options "-mmvcle -mzarch -dP" } */
+
+#include <string.h>
+
+void test(char *p, char c)
+{
+  __builtin_memset (p, c, 10000);
+}
+
+void test2(char *p, int c)
+{
+  __builtin_memset (p, (char)c, 10000);
+}
+
+/* Check that the right patterns are used.  */
+/* { dg-final { scan-assembler-times "c:10 .\*\{\\*setmem_long_31z\}" 1 } } */
+/* { dg-final { scan-assembler-times "c:15 .\*\{\\*setmem_long_31z\}" 1 } } */
-- 
2.3.0


[-- Attachment #4: 0001b-v2-S-390-Test-that-the-setmem_long_and-pattern-is-used.patch --]
[-- Type: text/x-diff, Size: 1991 bytes --]

From 0b14553b94c2a2b27bfbcdfadd483c57694c9127 Mon Sep 17 00:00:00 2001
From: Dominik Vogt <vogt@linux.vnet.ibm.com>
Date: Mon, 30 Nov 2015 15:34:27 +0100
Subject: [PATCH 1.5/1.5] S/390: Test that the setmem_long_and pattern is used.

---
 gcc/testsuite/gcc.target/s390/md/setmem_long-1.c | 7 ++++++-
 gcc/testsuite/gcc.target/s390/md/setmem_long-2.c | 6 +++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/gcc.target/s390/md/setmem_long-1.c b/gcc/testsuite/gcc.target/s390/md/setmem_long-1.c
index 9a926ce..c0ed482 100644
--- a/gcc/testsuite/gcc.target/s390/md/setmem_long-1.c
+++ b/gcc/testsuite/gcc.target/s390/md/setmem_long-1.c
@@ -17,4 +17,9 @@ void test2(char *p, int c)
 
 /* Check that the right patterns are used.  */
 /* { dg-final { scan-assembler-times "c:10 .\*\{\\*setmem_long\}" 1 } } */
-/* { dg-final { scan-assembler-times "c:15 .\*\{\\*setmem_long\}" 1 } } */
+/* { dg-final { scan-assembler-times "c:15 .\*\{\\*setmem_long_and\}" 1 } } */
+
+/* Check that the setmem_long_and pattern is used properly.  */
+/* { dg-final { scan-assembler-not "\tsllg\t" } } */
+/* { dg-final { scan-assembler-not "\tllgcr\t" } } */
+/* { dg-final { scan-assembler-not "\tn\t" } } */
diff --git a/gcc/testsuite/gcc.target/s390/md/setmem_long-2.c b/gcc/testsuite/gcc.target/s390/md/setmem_long-2.c
index ce6aa42..8f29071 100644
--- a/gcc/testsuite/gcc.target/s390/md/setmem_long-2.c
+++ b/gcc/testsuite/gcc.target/s390/md/setmem_long-2.c
@@ -17,4 +17,8 @@ void test2(char *p, int c)
 
 /* Check that the right patterns are used.  */
 /* { dg-final { scan-assembler-times "c:10 .\*\{\\*setmem_long_31z\}" 1 } } */
-/* { dg-final { scan-assembler-times "c:15 .\*\{\\*setmem_long_31z\}" 1 } } */
+/* { dg-final { scan-assembler-times "c:15 .\*\{\\*setmem_long_and\}" 1 } } */
+
+/* Check that the setmem_long_and pattern is used properly.  */
+/* { dg-final { scan-assembler-not "\tllcr\t" } } */
+/* { dg-final { scan-assembler-not "\tn\t" } } */
-- 
2.3.0