From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 127367 invoked by alias); 15 Nov 2016 18:05:24 -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 127348 invoked by uid 89); 15 Nov 2016 18:05:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.0 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW,RCVD_IN_SEMBACKSCATTER autolearn=no version=3.3.2 spammy=thorough, incoming, held X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0a-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.156.1) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 15 Nov 2016 18:05:13 +0000 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uAFI4UCi063192 for ; Tue, 15 Nov 2016 13:05:12 -0500 Received: from e35.co.us.ibm.com (e35.co.us.ibm.com [32.97.110.153]) by mx0a-001b2d01.pphosted.com with ESMTP id 26r1yxgp4y-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 15 Nov 2016 13:05:11 -0500 Received: from localhost by e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Nov 2016 11:05:11 -0700 Received: from d03dlp03.boulder.ibm.com (9.17.202.179) by e35.co.us.ibm.com (192.168.1.135) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 15 Nov 2016 11:05:09 -0700 Received: from b01cxnp22036.gho.pok.ibm.com (b01cxnp22036.gho.pok.ibm.com [9.57.198.26]) by d03dlp03.boulder.ibm.com (Postfix) with ESMTP id 7DB4219D804E; Tue, 15 Nov 2016 11:04:31 -0700 (MST) Received: from b01ledav005.gho.pok.ibm.com (b01ledav005.gho.pok.ibm.com [9.57.199.110]) by b01cxnp22036.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id uAFI50N131850604; Tue, 15 Nov 2016 18:05:08 GMT Received: from b01ledav005.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5DFE5AE043; Tue, 15 Nov 2016 13:05:08 -0500 (EST) Received: from oc6462846008.ibm.com (unknown [9.85.150.229]) by b01ledav005.gho.pok.ibm.com (Postfix) with ESMTP id 29665AE063; Tue, 15 Nov 2016 13:05:08 -0500 (EST) Subject: Re: [PATCH,rs6000] Add built-in function support for Power9 byte instructions To: gcc-patches@gcc.gnu.org References: <20161115111912.GC12325@gate.crashing.org> Cc: Segher Boessenkool From: Kelvin Nilsen Date: Tue, 15 Nov 2016 18:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <20161115111912.GC12325@gate.crashing.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16111518-0012-0000-0000-000011263281 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006083; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000189; SDB=6.00781090; UDB=6.00376738; IPR=6.00558599; BA=6.00004883; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013334; XFM=3.00000011; UTC=2016-11-15 18:05:11 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16111518-0013-0000-0000-000047325EB7 Message-Id: <71978567-5cfd-9d0c-7817-fee44f079874@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2016-11-15_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1611150311 X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg01510.txt.bz2 Thank you very much for the prompt and thorough review. There are a few points below where I'd like to seek further clarification. On 11/15/2016 04:19 AM, Segher Boessenkool wrote: > Hi! > > On Mon, Nov 14, 2016 at 04:43:35PM -0700, Kelvin Nilsen wrote: >> * config/rs6000/altivec.md (UNSPEC_CMPRB): New unspec value. >> (UNSPEC_CMPRB2): New unspec value. > > I wonder if you really need both? The number of arguments will tell > which is which, anyway? I appreciate your preference to avoid proliferation of special-case unspec constants. However, it is a not so straightforward to combine these two cases under the same constant value. The issue is that though the two encoding conceptually represent different "numbers of arguments", the arguments are all packed inside of a 32-bit register. At the RTL level, it looks like the two different forms have the same number of arguments (the same number of register operands). The difference is which bits serve relevant purposes within the incoming register operands. So I'm inclined to keep this as is if that's ok with you. > >> (cmprb_p): New expansion. > > Not such a great name (now you get a gen_cmprb_p function which isn't > a predicate itself). I'll change these names. > >> (CMPRB): Add byte-in-range built-in function. >> (CMBRB2): Add byte-in-either_range built-in function. >> (CMPEQB): Add byte-in-set builtin-in function. > > "builtin-in", and you typoed an underscore? Thanks. > >> +;; Predicate: test byte within range. >> +;; Return in target register operand 0 a non-zero value iff the byte >> +;; held in bits 24:31 of operand 1 is within the inclusive range >> +;; bounded below by operand 2's bits 0:7 and above by operand 2's >> +;; bits 8:15. >> +(define_expand "cmprb_p" > > It seems you got the bit numbers mixed up. Maybe just call it the low > byte, and the byte just above? > > (And it always sets 0 or 1 here, you might want to make that more explicit). > >> +;; Set bit 1 (the GT bit, 0x2) of CR register operand 0 to 1 iff the > > That's 4, i.e. 0b0100. > >> +;; Set operand 0 register to non-zero value iff the CR register named >> +;; by operand 1 has its GT bit (0x2) or its LT bit (0x1) set. >> +(define_insn "*setb" > > LT is 8, GT is 4. If LT is set it returns -1, otherwise if GT is set it > returns 1, otherwise it returns 0. > Thanks for catching this. I think I got endian confusion inside my head while I was writing the above. I will rewrite these comments, below also. >> +;; Predicate: test byte within two ranges. >> +;; Return in target register operand 0 a non-zero value iff the byte >> +;; held in bits 24:31 of operand 1 is within the inclusive range >> +;; bounded below by operand 2's bits 0:7 and above by operand 2's >> +;; bits 8:15 or if the byte is within the inclusive range bounded >> +;; below by operand 2's bits 16:23 and above by operand 2's bits 24:31. >> +(define_expand "cmprb2_p" > > The high bound is higher in the reg than the low bound. See the example > where 0x3930 is used to do isdigit (and yes 0x3039 would be much more > fun, but alas). > >> +;; Predicate: test byte membership within set of 8 bytes. >> +;; Return in target register operand 0 a non-zero value iff the byte >> +;; held in bits 24:31 of operand 1 equals at least one of the eight >> +;; byte values represented by the 64-bit register supplied as operand >> +;; 2. Note that the 8 byte values held within operand 2 need not be >> +;; unique. > > (trailing space) > > I wonder if we really need all these predicate expanders, if it wouldn't > be easier if the builtin handling code did the setb itself? > The reason it seems most "natural" to me use the expanders is because I need to introduce a temporary CR scratch register between expansion and insn matching. Also, it seems that the *setb pattern may be of more general use in the future implementation of other built-in functions. I'm inclined to keep this as is, but if you still feel otherwise, I'll figure out how to avoid the expansion.