From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 126974 invoked by alias); 22 Jul 2019 18:59:50 -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 126965 invoked by uid 89); 22 Jul 2019 18:59:49 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.1 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.1 spammy= 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; Mon, 22 Jul 2019 18:59:47 +0000 Received: from pps.filterd (m0098399.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id x6MIiX7u082169 for ; Mon, 22 Jul 2019 14:59:46 -0400 Received: from e13.ny.us.ibm.com (e13.ny.us.ibm.com [129.33.205.203]) by mx0a-001b2d01.pphosted.com with ESMTP id 2twhpsjsfs-1 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 22 Jul 2019 14:59:46 -0400 Received: from localhost by e13.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 22 Jul 2019 19:59:45 +0100 Received: from b01cxnp22035.gho.pok.ibm.com (9.57.198.25) by e13.ny.us.ibm.com (146.89.104.200) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; (version=TLSv1/SSLv3 cipher=AES256-GCM-SHA384 bits=256/256) Mon, 22 Jul 2019 19:59:42 +0100 Received: from b01ledav006.gho.pok.ibm.com (b01ledav006.gho.pok.ibm.com [9.57.199.111]) by b01cxnp22035.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id x6MIxf7w54526294 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Mon, 22 Jul 2019 18:59:41 GMT Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3A51CAC066; Mon, 22 Jul 2019 18:59:41 +0000 (GMT) Received: from b01ledav006.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E6FD7AC05B; Mon, 22 Jul 2019 18:59:40 +0000 (GMT) Received: from ibm-toto.the-meissners.org (unknown [9.32.77.177]) by b01ledav006.gho.pok.ibm.com (Postfix) with ESMTPS; Mon, 22 Jul 2019 18:59:40 +0000 (GMT) Date: Mon, 22 Jul 2019 19:33:00 -0000 From: Michael Meissner To: Segher Boessenkool Cc: Michael Meissner , gcc-patches@gcc.gnu.org, dje.gcc@gmail.com Subject: Re: [PATCH], Patch #10, move PowerPC data structures & helper functions from rs6000.c to rs6000-internal.h Mail-Followup-To: Michael Meissner , Segher Boessenkool , gcc-patches@gcc.gnu.org, dje.gcc@gmail.com References: <20190628000602.GA24286@ibm-toto.the-meissners.org> <20190720161308.GA6730@ibm-toto.the-meissners.org> <20190722054213.GV20882@gate.crashing.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190722054213.GV20882@gate.crashing.org> User-Agent: Mutt/1.5.21 (2010-09-15) x-cbid: 19072218-0064-0000-0000-000004020A13 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00011476; HX=3.00000242; KW=3.00000007; PH=3.00000004; SC=3.00000287; SDB=6.01235940; UDB=6.00651373; IPR=6.01017291; MB=3.00027841; MTD=3.00000008; XFM=3.00000015; UTC=2019-07-22 18:59:43 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 19072218-0065-0000-0000-00003E605016 Message-Id: <20190722185939.GC30230@ibm-toto.the-meissners.org> X-SW-Source: 2019-07/txt/msg01444.txt.bz2 On Mon, Jul 22, 2019 at 12:42:13AM -0500, Segher Boessenkool wrote: > Hi! > > On Sat, Jul 20, 2019 at 12:13:08PM -0400, Michael Meissner wrote: > > 2019-07-20 Michael Meissner > > > > * config/rs6000/rs6000-internal.h (rs6000_hard_regno_mode_ok_p): > > Move various declarations relating to addressing and register > > allocation to rs6000-internal.h from rs6000.c so that in the > > future we can move things out of rs6000.c. > > Just say > (rs6000_hard_regno_mode_ok_p): New declaration. > for the things that only had a definition before. > > > Make the static arrays global, > > That's not this entry. Say that in the entries where it applies. > > > and define them in rs6000.c. > > Say that in the corresponding entry for rs6000.c . > > > (enum rs6000_reg_type): Likewise. > > This one always was a declaration. This was a mechanical patch, just moving the swath of code from rs6000.c to rs6000-internal.h. > (... ten gazillion "Likewise." ...) > Most of those are *not* the same thing. Don't say "likewise" if not > the same comment applies. > > If it is hard to write a proper changelog, your patch series probably > could use some restructuring. Or sometimes the changelog you need just > is more work than you would prefer. > > You don't necessarily have to keep the same order in the changelog as > in the patch, if that helps. But roughly the same order helps review, > so please consider that too ;-) Well yes, while in general I would prefer to group things logically together for ChangeLog, I don't tend to do it because as you say it makes it easier for the review. > We haven't had GO_IF_LEGITIMATE_ADDRESS for ten years now, please don't > introduce new references to it ;-) Yep. > > +#define RELOAD_REG_VALID 0x0001 /* Mode valid in register.. */ > > +#define RELOAD_REG_MULTIPLE 0x0002 /* Mode takes multiple registers. */ > > +#define RELOAD_REG_INDEXED 0x0004 /* Reg+reg addressing. */ > > +#define RELOAD_REG_OFFSET 0x0008 /* Reg+offset addressing. */ > > +#define RELOAD_REG_PRE_INCDEC 0x0010 /* PRE_INC/PRE_DEC valid. */ > > +#define RELOAD_REG_PRE_MODIFY 0x0020 /* PRE_MODIFY valid. */ > > +#define RELOAD_REG_AND_M16 0x0040 /* AND -16 addressing. */ > > +#define RELOAD_REG_QUAD_OFFSET 0x0080 /* quad offset is limited. */ > > Why all the extra zeroes? If you introduce some 0x100 later, just leave > the 0x80 as 0x80 please, that is much more readable. As I mentioned, I will be adding at least one new bit (RELOAD_REG_DS_OFFSET), but I have potential patches that add a few more (those are still in flux), and those would need the extra column. I personally DO NOT find mask definitions like: #define RELOAD_REG_VALID 0x01 /* Mode valid in register.. */ #define RELOAD_REG_MULTIPLE 0x02 /* Mode takes multiple registers. */ #define RELOAD_REG_INDEXED 0x04 /* Reg+reg addressing. */ #define RELOAD_REG_OFFSET 0x08 /* Reg+offset addressing. */ #define RELOAD_REG_PRE_INCDEC 0x10 /* PRE_INC/PRE_DEC valid. */ #define RELOAD_REG_PRE_MODIFY 0x20 /* PRE_MODIFY valid. */ #define RELOAD_REG_AND_M16 0x40 /* AND -16 addressing. */ #define RELOAD_REG_QUAD_OFFSET 0x80 /* quad offset is limited. */ #define RELOAD_REG_DS_OFFSET 0x100 /* bottom 2 bits must be zero. */ to be readable. So in change, I can go back to just: #define RELOAD_REG_VALID 0x01 /* Mode valid in register.. */ #define RELOAD_REG_MULTIPLE 0x02 /* Mode takes multiple registers. */ #define RELOAD_REG_INDEXED 0x04 /* Reg+reg addressing. */ #define RELOAD_REG_OFFSET 0x08 /* Reg+offset addressing. */ #define RELOAD_REG_PRE_INCDEC 0x10 /* PRE_INC/PRE_DEC valid. */ #define RELOAD_REG_PRE_MODIFY 0x20 /* PRE_MODIFY valid. */ #define RELOAD_REG_AND_M16 0x40 /* AND -16 addressing. */ #define RELOAD_REG_QUAD_OFFSET 0x80 /* quad offset is limited. */ And in the next patch, change it all to: #define RELOAD_REG_VALID 0x001 /* Mode valid in register.. */ #define RELOAD_REG_MULTIPLE 0x002 /* Mode takes multiple registers. */ #define RELOAD_REG_INDEXED 0x004 /* Reg+reg addressing. */ #define RELOAD_REG_OFFSET 0x008 /* Reg+offset addressing. */ #define RELOAD_REG_PRE_INCDEC 0x010 /* PRE_INC/PRE_DEC valid. */ #define RELOAD_REG_PRE_MODIFY 0x020 /* PRE_MODIFY valid. */ #define RELOAD_REG_AND_M16 0x040 /* AND -16 addressing. */ #define RELOAD_REG_QUAD_OFFSET 0x080 /* quad offset is limited. */ #define RELOAD_REG_DS_OFFSET 0x100 /* bottom 2 bits must be 0. */ Some time later if I continue with the current code, that would be changed to: #define RELOAD_REG_VALID 0x0001 /* Mode valid in register.. */ #define RELOAD_REG_MULTIPLE 0x0002 /* Mode takes multiple registers. */ #define RELOAD_REG_INDEXED 0x0004 /* Reg+reg addressing. */ #define RELOAD_REG_OFFSET 0x0008 /* Reg+offset addressing. */ #define RELOAD_REG_PRE_INCDEC 0x0010 /* PRE_INC/PRE_DEC valid. */ #define RELOAD_REG_PRE_MODIFY 0x0020 /* PRE_MODIFY valid. */ #define RELOAD_REG_AND_M16 0x0040 /* AND -16 addressing. */ #define RELOAD_REG_QUAD_OFFSET 0x0080 /* DQ-form (bottom 4 bits 0). */ #define RELOAD_REG_DS_OFFSET 0x0100 /* DS-form (bottom 2 bits 0). */ /* The following are flags used in address decoding, but are not currently stored in the addr_mask field of reg_addr. */ #define RELOAD_REG_SINGLE 0x0200 /* Address is a single register. */ #define RELOAD_REG_IS_PREFIXED 0x0400 /* Address uses prefix encoding. */ #define RELOAD_REG_NOT_PREFIXED 0x0800 /* Address does not use a prefix. */ #define RELOAD_REG_PCREL_LOCAL 0x1000 /* Pc-relative to local symbol. */ #define RELOAD_REG_PCREL_EXT 0x2000 /* Pc-relative to external symbol. */ #define RELOAD_REG_LO_SUM 0x4000 /* Address uses LO_SUM (TOC, etc). */ I was just trying to save a step since I was moving the definitions any way to add the alignment. > > It's hard to tell whether the problem is factored sanely, or if this > creates a big mountain of spaghetti instead. Can you show how this is > used later? The intention is to move bits to other files. In particular, I want to create a new rs6000-prefixed.c file that contains the bits specific to prefixed addressing. However, a lot of the bits need access to the reg_addr array. When you have the reg_addr array, all of the mode_supports_* functions also become useful in the other files. In addition to reg_array, the code that determines if something is a traditional instruction or a prefixed instruction needs the register type classificiation (rs6000_reg_type). The issue is in some instructions it depends on whether the offset is an instruction using the DS encoding or D encoding. For example, loading floating point scalars to a traditional FPR register uses the D encoding, but loading floating point scalars to an altivec register uses the DS encoding. However, longer term, it would be nice to move other things from rs6000.c to other files, such as the functions that deal with p8 fusion, and the legitimate address functions (which I still tend to mentally classify as GO_IF_LEGITIMATE type functions, even as you mention, we no longer have GO_IF_LEGITIMATE*). > Normally, you send a whole series, and then perhaps many of the first > are preparatory only, but a reviewer can see where things are headed, > and *then* simple refactorings like this can make sense. The way this > patch looks now you are just making a lot of data global. This was intended to be just a mechanical patch to move things to the .h file. -- Michael Meissner, IBM IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA email: meissner@linux.ibm.com, phone: +1 (978) 899-4797