Re: [PATCH] rtl: add predicates for addition, subtraction & multiplication
On Sun, Nov 27, 2022 at 09:21:00AM -0500, David Malcolm via Gcc-patches wrote: > We're currently in "stage 3" of GCC 13 development, which means that > we're focusing on bug-fixing, rather than cleanups and feature work. > Though exceptions can be made for low-risk work, at the discretion of > the release managers; I've taken the liberty of CCing them. Such global changes are incomnvenient for people who have touched any of that code in their own patches. If we really want to do that it should be done early in stage 1 (when everything is broken for everyone anyway), and should be agreed on beforehand, or really, should only be done for obvious improvements. This is not an obvious improvement. > > All existings tests did pass. I have never seen a single target where all existing tests passed. What we usually do is "same failures before and after the patch" :-) > RTL is an aspect of the compiler that tends to have the most per-target > differences, so it's especially important to be precise about which > target(s) you built and tested on. Not that that should matter at all for patches that do not actually change anything, like this one should be: it should only change notation. That is in the nature of helper functions and helper macros. > > Like I said, this is my first patch. > > We're sometimes not as welcoming to newcomers as we could be, so please > bear with us. Let me know if anything in this email is unclear. x2 from me! > As noted in another reply, there are lots of places in the code where > the patch touches lines that it doesn't need to: generally formatting > and whitespace changes. > > We have over 30 years of source history which we sometimes need to look > back on, and RTL is some of the oldest code in the compiler, so we want > to minimize "churn" to keep tools like "git blame" useful. Not to mention that many of those changes violated our coding style, or even look like an automated formatter going haywire. And, of course, such changes should be separate patches, if done at all! Segher
Re: [PATCH] rtl: add predicates for addition, subtraction & multiplication
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) > -
Re: [PATCH] rtl: add predicates for addition, subtraction & multiplication
On Sat, 2022-11-26 at 21:16 -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. I > added predicates for addition, subtraction and multiplication. I also > went through and used them in the code. > > I did not add tests because I'm not addding/modifying any behavior. > All existings tests did pass. > > Like I said, this is my first patch. Please let me know if I did > anything wrong or if there's anything I can improve for next time. > > Signed-off-by: Charlie Sale > --- > gcc/ChangeLog | 43 +++ > gcc/alias.cc | 30 +++-- > gcc/auto-inc-dec.cc | 11 +- > gcc/calls.cc | 8 +- > gcc/cfgexpand.cc | 16 +-- > gcc/combine-stack-adj.cc | 39 +++ > gcc/combine.cc | 241 +-- > gcc/compare-elim.cc | 3 +- > gcc/cse.cc | 66 +-- > gcc/cselib.cc | 37 +++--- > gcc/dce.cc | 4 +- > gcc/dwarf2cfi.cc | 2 +- > gcc/dwarf2out.cc | 11 +- > gcc/emit-rtl.cc | 6 +- > gcc/explow.cc | 31 ++--- > gcc/expr.cc | 23 ++-- > gcc/final.cc | 20 ++-- > gcc/function.cc | 7 +- > gcc/fwprop.cc | 2 +- > gcc/haifa-sched.cc | 10 +- > gcc/ifcvt.cc | 11 +- > gcc/ira.cc | 6 +- > gcc/loop-doloop.cc | 70 ++-- > gcc/loop-iv.cc | 21 +--- > gcc/lra-constraints.cc | 34 +++--- > gcc/lra-eliminations.cc | 25 ++-- > gcc/lra.cc | 6 +- > gcc/modulo-sched.cc | 2 +- > gcc/postreload.cc | 25 ++-- > gcc/reginfo.cc | 12 +- > gcc/reload.cc | 180 + > gcc/reload1.cc | 85 ++ > gcc/reorg.cc | 12 +- > gcc/rtl.cc | 3 +- > gcc/rtl.h | 11 ++ > gcc/rtlanal.cc | 25 ++-- > gcc/sched-deps.cc | 8 +- > gcc/simplify-rtx.cc | 143 +-- > gcc/var-tracking.cc | 37 +++--- > 39 files changed, 595 insertions(+), 731 deletions(-) > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog Do not edit this file. It's automatically generated from Git commit log. Write the ChangeLog entries in the commit message instead. /* snip */ > - return simplify_gen_binary (GET_CODE (x), GET_MODE (x), > - op0, XEXP (x, 1)); > + return simplify_gen_binary (GET_CODE (x), GET_MODE (x), op0, > + XEXP (x, 1)); Don't update things irrelevant to your topic stealthy. /* snip */ > - && (t = get_reg_known_value (REGNO (XEXP (src, > 0 > + && (t = get_reg_known_value ( > + REGNO (XEXP (src, 0 Likewise. /* snip */ > - op0 = expand_debug_expr (TREE_OPERAND (TREE_OPERAND (exp, 0), > - 0)); > + op0 = expand_debug_expr (TREE_OPERAND (TREE_OPERAND (exp, 0), > 0)); Likewise. /* snip */ > - else if (dest == stack_pointer_rtx > - && REG_P (src) > + else if (dest == stack_pointer_rtx && REG_P (src) Likewise, and GCC has its own coding style which is not "what looks better in your opinion". I'll stop reading the patch here because it's too long and there is already too much stealth changes. Try to break the patch into a series of multiple small patches because it's difficult to review a very large diff. Do not randomly modify coding style. Even if you have a good reason to make style changes, you should submit them as a separate patch (or separate patch series) because mixing style changes and real code changes makes it difficult to review the code. -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University
[PATCH] rtl: add predicates for addition, subtraction & multiplication
This is my first contribution to GCC :) one of the beginner projects
suggested on the website was to add and use RTL type predicates. I
added predicates for addition, subtraction and multiplication. I also
went through and used them in the code.
I did not add tests because I'm not addding/modifying any behavior.
All existings tests did pass.
Like I said, this is my first patch. Please let me know if I did
anything wrong or if there's anything I can improve for next time.
Signed-off-by: Charlie Sale
---
gcc/ChangeLog| 43 +++
gcc/alias.cc | 30 +++--
gcc/auto-inc-dec.cc | 11 +-
gcc/calls.cc | 8 +-
gcc/cfgexpand.cc | 16 +--
gcc/combine-stack-adj.cc | 39 +++
gcc/combine.cc | 241 +--
gcc/compare-elim.cc | 3 +-
gcc/cse.cc | 66 +--
gcc/cselib.cc| 37 +++---
gcc/dce.cc | 4 +-
gcc/dwarf2cfi.cc | 2 +-
gcc/dwarf2out.cc | 11 +-
gcc/emit-rtl.cc | 6 +-
gcc/explow.cc| 31 ++---
gcc/expr.cc | 23 ++--
gcc/final.cc | 20 ++--
gcc/function.cc | 7 +-
gcc/fwprop.cc| 2 +-
gcc/haifa-sched.cc | 10 +-
gcc/ifcvt.cc | 11 +-
gcc/ira.cc | 6 +-
gcc/loop-doloop.cc | 70 ++--
gcc/loop-iv.cc | 21 +---
gcc/lra-constraints.cc | 34 +++---
gcc/lra-eliminations.cc | 25 ++--
gcc/lra.cc | 6 +-
gcc/modulo-sched.cc | 2 +-
gcc/postreload.cc| 25 ++--
gcc/reginfo.cc | 12 +-
gcc/reload.cc| 180 +
gcc/reload1.cc | 85 ++
gcc/reorg.cc | 12 +-
gcc/rtl.cc | 3 +-
gcc/rtl.h| 11 ++
gcc/rtlanal.cc | 25 ++--
gcc/sched-deps.cc| 8 +-
gcc/simplify-rtx.cc | 143 +--
gcc/var-tracking.cc | 37 +++---
39 files changed, 595 insertions(+), 731 deletions(-)
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index f999e2cba43..1fd2c94c873 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,46 @@
+2022-11-26 Charlie Sale
+
+ * rtl.h (PLUS_P): RTL addition predicate
+ (MINUS_P): RTL subtraction predicate
+ (MULT_P): RTL multiplication predicate
+ * alias.cc: use RTL predicates
+ * auto-inc-dec.cc: use RTL predicates
+ * calls.cc: use RTL predicates
+ * cfgexpand.cc: use RTL predicates
+ * combine-stack-adj.cc: use RTL predicates
+ * combine.cc: use RTL predicates
+ * compare-elim.cc: use RTL predicates
+ * cse.cc: use RTL predicates
+ * cselib.cc: use RTL predicates
+ * dce.cc: use RTL predicates
+ * dwarf2cfi.cc: use RTL predicates
+ * dwarf2out.cc: use RTL predicates
+ * emit-rtl.cc: use RTL predicates
+ * explow.cc: use RTL predicates
+ * expr.cc: use RTL predicates
+ * final.cc: use RTL predicates
+ * function.cc: use RTL predicates
+ * fwprop.cc: use RTL predicates
+ * haifa-sched.cc: use RTL predicates
+ * ifcvt.cc: use RTL predicates
+ * ira.cc: use RTL predicates
+ * loop-doloop.cc: use RTL predicates
+ * loop-iv.cc: use RTL predicates
+ * lra-constraints.cc: use RTL predicates
+ * lra-eliminations.cc: use RTL predicates
+ * lra.cc: use RTL predicates
+ * modulo-sched.cc: use RTL predicates
+ * postreload.cc: use RTL predicates
+ * reginfo.cc: use RTL predicates
+ * reload.cc: use RTL predicates
+ * reload1.cc: use RTL predicates
+ * reorg.cc: use RTL predicates
+ * rtl.cc: use RTL predicates
+ * rtlanal.cc: use RTL predicates
+ * sched-deps.cc: use RTL predicates
+ * simplify-rtx.cc: use RTL predicates
+ * var-tracking.cc: use RTL predicates
+
2022-11-25 Sandra Loosemore
* common.opt (fopenmp-target-simd-clone): New option.
diff --git a/gcc/alias.cc b/gcc/alias.cc
index c62837dd854..2d9bd79fe21 100644
--- a/gcc/alias.cc
+++ b/gcc/alias.cc
@@ -1473,7 +1473,7 @@ find_base_value (rtx src)
otherwise. */
if (copying_arguments
&& (XEXP (src, 0) == arg_pointer_rtx
- || (GET_CODE (XEXP (src, 0)) == PLUS
+ || (PLUS_P (XEXP (src, 0))
&& XEXP (XEXP (src, 0), 0) == arg_pointer_rtx)))
return arg_base_value;
return 0;
@@ -1790,7 +1790,7 @@ canon_rtx (rtx x)
return canon_rtx (t);
}
- if (GET_CODE (x) == PLUS)
+ if (PLUS_P (x))
{
rtx x0 = canon_rtx (XEXP (x, 0));
rtx x1 = canon_rtx (XEXP (x, 1));
@@ -2357,19 +2357,17 @@ get_addr (rtx x)
if (GET_CODE (x) != VALUE)
{
- if ((GET_CODE (x) == PLUS || GET_CODE (x) == MINUS)
- && GET_CODE (XEXP (x, 0)) == VALUE
+ if ((PLUS_P (x) || MINUS_P (x)) && GET_CODE (XEXP (x, 0)) == VALUE
