From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 5E9C23858434 for ; Tue, 29 Nov 2022 00:10:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5E9C23858434 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 2AT09OHX020557; Mon, 28 Nov 2022 18:09:24 -0600 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 2AT09OJG020556; Mon, 28 Nov 2022 18:09:24 -0600 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Mon, 28 Nov 2022 18:09:24 -0600 From: Segher Boessenkool To: Charlie Sale Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] rtl: add predicates for addition, subtraction & multiplication Message-ID: <20221129000924.GQ25951@gate.crashing.org> References: <20221127021613.432881-1-softwaresale01@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20221127021613.432881-1-softwaresale01@gmail.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi! On Sat, Nov 26, 2022 at 09:16:13PM -0500, Charlie Sale via Gcc-patches wrote: > This is my first contribution to GCC :) one of the beginner projects > suggested on the website was to add and use RTL type predicates. It says "See which ones are worth having a predicate for, and add them." None of the operations should get a predicate, imnsho, only more structural things. Code using PLUS_P is way *less* readable than that using GET_CODE directly! It is good if important things are more explicit, it is bad to have many names, etc. > + * rtl.h (PLUS_P): RTL addition predicate > + (MINUS_P): RTL subtraction predicate > + (MULT_P): RTL multiplication predicate * rtl.h (PLUS_P): New. > + * alias.cc: use RTL predicates * alias.cc: Use new predicates. Send the changelog as plain text btw, not as patch; if nothing else, such patches will never apply cleanly :-) > set_reg_known_value (regno, XEXP (note, 0)); > - set_reg_known_equiv_p (regno, > - REG_NOTE_KIND (note) == REG_EQUIV); > + set_reg_known_equiv_p (regno, REG_NOTE_KIND (note) > + == REG_EQUIV); Don't reformat unrelated code. And certainly not to something that violates our coding standards :-) > - && (t = get_reg_known_value (REGNO (XEXP (src, 0)))) > + && (t = get_reg_known_value ( > + REGNO (XEXP (src, 0)))) Wow, this is even worse. Why would you do this at all? I guess you used some automatic formatting thing that sets maximum line length to 79? It is 80, and it is a bad idea to reformat any random code. > - && (REG_P (XEXP (SET_SRC (pat), 1))) > - && GET_CODE (SET_SRC (pat)) == PLUS) > + && (REG_P (XEXP (SET_SRC (pat), 1))) && PLUS_P (SET_SRC (pat))) You could have removed the superfluous extra parentheses here :-) > case SUBREG: > if ((SUBREG_PROMOTED_VAR_P (x) > || (REG_P (SUBREG_REG (x)) && REG_POINTER (SUBREG_REG (x))) > - || (GET_CODE (SUBREG_REG (x)) == PLUS > - && REG_P (XEXP (SUBREG_REG (x), 0)) > + || (PLUS_P (SUBREG_REG (x)) && REG_P (XEXP (SUBREG_REG (x), 0)) > && REG_POINTER (XEXP (SUBREG_REG (x), 0)) > && CONST_INT_P (XEXP (SUBREG_REG (x), 1)))) There was only one && per line here on purpose. It makes the code much more readable. > - if (GET_CODE (x) == PLUS > - && XEXP (x, 0) == stack_pointer_rtx > + if (PLUS_P (x) && XEXP (x, 0) == stack_pointer_rtx > && CONST_INT_P (XEXP (x, 1))) Similar here (but it is so simple here that either is easy to read of course). > --- a/gcc/combine.cc > +++ b/gcc/combine.cc > @@ -3016,19 +3016,17 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0, > /* See if any of the insns is a MULT operation. Unless one is, we will > reject a combination that is, since it must be slower. Be conservative > here. */ > - if (GET_CODE (i2src) == MULT > - || (i1 != 0 && GET_CODE (i1src) == MULT) > - || (i0 != 0 && GET_CODE (i0src) == MULT) > - || (GET_CODE (PATTERN (i3)) == SET > - && GET_CODE (SET_SRC (PATTERN (i3))) == MULT)) > + if (MULT_P (i2src) || (i1 != 0 && MULT_P (i1src)) > + || (i0 != 0 && MULT_P (i0src)) > + || (GET_CODE (PATTERN (i3)) == SET && MULT_P (SET_SRC (PATTERN (i3))))) > have_mult = 1; No. All || align here. Please leave it that way. > - /* If I3 has an inc, then give up if I1 or I2 uses the reg that is inc'd. > - We used to do this EXCEPT in one case: I3 has a post-inc in an > - output operand. However, that exception can give rise to insns like > - mov r3,(r3)+ > - which is a famous insn on the PDP-11 where the value of r3 used as the > - source was model-dependent. Avoid this sort of thing. */ > + /* If I3 has an inc, then give up if I1 or I2 uses the reg that is inc'd. > + We used to do this EXCEPT in one case: I3 has a post-inc in an > + output operand. However, that exception can give rise to insns like > + mov r3,(r3)+ > + which is a famous insn on the PDP-11 where the value of r3 used as the > + source was model-dependent. Avoid this sort of thing. */ The indentation was correct, it now isn't anymore. There is absolutely no reason to touch this at all anyway. NAK. > - if ((FIND_REG_INC_NOTE (i2, NULL_RTX) != 0 > - && i2_is_used + added_sets_2 > 1) > + if ((FIND_REG_INC_NOTE (i2, NULL_RTX) != 0 && i2_is_used + added_sets_2 > 1) Do not touch random other code please. If there is a reason to reformat it (there isn't here!) do that as a separate patch. > || (i1 != 0 && FIND_REG_INC_NOTE (i1, NULL_RTX) != 0 > - && (i1_is_used + added_sets_1 + (added_sets_2 && i1_feeds_i2_n) > - > 1)) > + && (i1_is_used + added_sets_1 + (added_sets_2 && i1_feeds_i2_n) > 1)) > || (i0 != 0 && FIND_REG_INC_NOTE (i0, NULL_RTX) != 0 > - && (n_occurrences + added_sets_0 > - + (added_sets_1 && i0_feeds_i1_n) > - + (added_sets_2 && i0_feeds_i2_n) > + && (n_occurrences + added_sets_0 + (added_sets_1 && i0_feeds_i1_n) > + + (added_sets_2 && i0_feeds_i2_n) > > 1)) These are way worse than they were before, and also different layou than we use in GCC. NAK. > - if (GET_CODE (XEXP (x, 0)) == PLUS > - && CONST_INT_P (XEXP (XEXP (x, 0), 1)) > - && ! memory_address_addr_space_p (GET_MODE (x), XEXP (x, 0), > - MEM_ADDR_SPACE (x))) > + if (PLUS_P (XEXP (x, 0)) && CONST_INT_P (XEXP (XEXP (x, 0), 1)) > + && !memory_address_addr_space_p (GET_MODE (x), XEXP (x, 0), > + MEM_ADDR_SPACE (x))) combine.c (as well as other older code) uses space-after-bang a lot. Do not change that randomly please, because that a) makes it harder to review the code (separate patch please!), and b) it makes it harder to read the resulting code! Or did you change do s/! ([a-zA-A0-9_])/!\1/ here on the whole file? > - rtx_insn *seq = combine_split_insns (gen_rtx_SET (reg, XEXP (x, 0)), > - subst_insn); > + rtx_insn *seq > + = combine_split_insns (gen_rtx_SET (reg, XEXP (x, 0)), subst_insn); The original was better. For big mechanical patches, please do only *one* thing per patch, so all the patches in the series will be totally boring and much MUCH easier to review. But again, I think it is a mistake to make predicates for PLUS and MINUS and such. Sorry. PLUS is frequent in this patch, but MINUS and MULT are not at all, and even PLUS is not common in our code. A few characters shorter is nice for *very* common things, but extra expressiveness through verbosity is worth more here, imo. Segher