From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 99456 invoked by alias); 1 Feb 2016 13:30:28 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 99439 invoked by uid 89); 1 Feb 2016 13:30:27 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=dd, 390, PLUS, an X-HELO: e06smtp06.uk.ibm.com Received: from e06smtp06.uk.ibm.com (HELO e06smtp06.uk.ibm.com) (195.75.94.102) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Mon, 01 Feb 2016 13:30:26 +0000 Received: from localhost by e06smtp06.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 1 Feb 2016 13:30:23 -0000 Received: from d06dlp02.portsmouth.uk.ibm.com (9.149.20.14) by e06smtp06.uk.ibm.com (192.168.101.136) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 1 Feb 2016 13:30:22 -0000 X-IBM-Helo: d06dlp02.portsmouth.uk.ibm.com X-IBM-MailFrom: uweigand@de.ibm.com X-IBM-RcptTo: gcc-patches@gcc.gnu.org Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id CB6EC2190056 for ; Mon, 1 Feb 2016 13:30:08 +0000 (GMT) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u11DULeM9568674 for ; Mon, 1 Feb 2016 13:30:22 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u11DULL5004838 for ; Mon, 1 Feb 2016 08:30:21 -0500 Received: from oc7340732750.ibm.com (dyn-9-152-213-148.boeblingen.de.ibm.com [9.152.213.148]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id u11DULSY004815; Mon, 1 Feb 2016 08:30:21 -0500 Received: by oc7340732750.ibm.com (Postfix, from userid 500) id 56CA120D1; Mon, 1 Feb 2016 14:30:21 +0100 (CET) Subject: Re: [PATCH 3/9] S/390: Get rid of Y constraint in rotate patterns. To: krebbel@linux.vnet.ibm.com (Andreas Krebbel) Date: Mon, 01 Feb 2016 13:30:00 -0000 From: "Ulrich Weigand" Cc: gcc-patches@gcc.gnu.org In-Reply-To: <1452789254-12603-4-git-send-email-krebbel@linux.vnet.ibm.com> from "Andreas Krebbel" at Jan 14, 2016 05:33:28 PM MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-Id: <20160201133021.56CA120D1@oc7340732750.ibm.com> X-TM-AS-MML: disable X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16020113-0025-0000-0000-000005B608CA X-SW-Source: 2016-02/txt/msg00031.txt.bz2 Andreas Krebbel wrote: > +; Accept single register and immediate operands usable as shift > +; counts. > +(define_predicate "addrreg_or_constint_operand" > + (match_code "reg, subreg, const_int") I'm wondering whether this is even necessary. > +{ > + if (GET_MODE (op) != VOIDmode > + && GET_MODE_CLASS (GET_MODE (op)) != MODE_INT) > + return false; The predicate always seems to be used with a mode, so any extra mode checks here seem redundant. > + while (op && GET_CODE (op) == SUBREG) > + op = SUBREG_REG (op); > + > + if (REG_P (op) > + && (REGNO (op) >= FIRST_PSEUDO_REGISTER || ADDR_REG_P (op))) > + return true; > + > + if (CONST_INT_P (op)) > + return true; This looks somewhat copied from shift_count_or_setmem_operand, where we have a comment: /* Don't allow any non-base hard registers. Doing so without confusing reload and/or regrename would be tricky, and doesn't buy us much anyway. */ With the new set up, I don't believe there is any issue with confusing reload -- it's no more complex than any other pattern now. So it should be fine to just accept any register. In fact, I'm starting to wonder whether it wouldn't be just fine to simply using nonmemory_operand instead of this special predicate (the only issue might be that we could get CONST_WIDE_INT, but it should be simple to handle those). > +(define_expand "rotl3" > + [(set (match_operand:GPR 0 "register_operand" "") > + (rotate:GPR (match_operand:GPR 1 "register_operand" "") > + (match_operand:SI 2 "shift_count_or_setmem_operand" "")))] Similarly, I think the expander no longer needs to use the special predicate shift_count_or_setmem_operandm but could just use nonmemory_operand. [ This will reject the PLUS that shift_count_or_setmem_operand accepted, but I don't believe you ever *could* get the PLUS in the expander anyway. It will only be moved in by combine, so as long as the insn accepts it, everything should be good. ] > +(define_insn "*rotl3" > + [(set (match_operand:GPR 0 "register_operand" "=d,d") > + (rotate:GPR (match_operand:GPR 1 "register_operand" "d,d") > + (match_operand:SI 2 "addrreg_or_constint_operand" "a,n")))] > + "TARGET_CPU_ZARCH" > + "@ > + rll\t%0,%1,(%2) > + rll\t%0,%1,%Y2" So it looks like %Y is now always used for just a constant. Maybe the implementation of %Y should be adjusted (once the patch series is complete)? Also, if we use just nonmemory_operand, %Y should be updated to accept CONST_WIDE_INT just like e.g. %x does. > +; This expands an register/immediate operand to a register+immediate > +; operand to draw advantage of the address style operand format > +; providing a addition for free. > +(define_subst "addr_style_op_subst" > + [(set (match_operand:DSI 0 "" "") > + (SUBST:DSI (match_operand:DSI 1 "" "") > + (match_operand:SI 2 "" "")))] > + "REG_P (operands[2])" > + [(set (match_dup 0) > + (SUBST:DSI (match_dup 1) > + (plus:SI (match_dup 2) > + (match_operand 3 "const_int_operand" "n"))))]) This seems to add just a single alternative. Is that OK if it gets substituted into a pattern that used multiple alternatives otherwise? > +; In the subst pattern we have to disable the alternative where op2 is > +; already a constant. This attribute is supposed to be used in the > +; "disabled" insn attribute to achieve this. > +(define_subst_attr "addr_style_op_disable" "addr_style_op_subst" "0" "1") Is this necessary given that the subst adds a REG_P (operands[2]) condition? Bye, Ulrich -- Dr. Ulrich Weigand GNU/Linux compilers and toolchain Ulrich.Weigand@de.ibm.com