diff mbox

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

Message ID 20151201095954.GA31318@linux.vnet.ibm.com
State New
Headers show

Commit Message

Dominik Vogt Dec. 1, 2015, 9:59 a.m. UTC 
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 ^_^  ^_^

Comments

Dominik Vogt Dec. 1, 2015, 1:54 p.m. UTC | #1 
On Tue, Dec 01, 2015 at 10:59:54AM +0100, Dominik Vogt wrote:
> @@ -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")
                         ^^^^^^^^^^^^^^^

match_operand:SI

> +                     (subreg:P (match_dup 1) 8)] UNSPEC_REPLICATE_BYTE))
                         ^^^^^^^^
subreg:SI

> +   (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"

Ciao

Dominik ^_^  ^_^
diff mbox

Patch

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