diff mbox series

Reformat PowerPC movsi_internal

Message ID 20191127013212.GA31715@ibm-toto.the-meissners.org
State New
Headers show
Series Reformat PowerPC movsi_internal | expand

Commit Message

Michael Meissner Nov. 27, 2019, 1:32 a.m. UTC
As we discussed in the V6 patches #1 and #2, before submitting the patches
adding eI support for movdi and movsi, you prefered that I reformat the patches
to make them easier in the future to identify the changes.

This patch changes just the movsi_internal insn.  All of the constraints are in
the same order as the current sources.  I did add setting "num_insns" to this
patch, but I left in setting "length".  I found otherwise the Spec benchmarks
did not generate the same code with the patch.

I have bootstrapped the compiler with this change and the movdi_internal64
patch that will be submitted next, and there were no problems with the
bootstrap or tests that regressed.  I compared Spec 2017 INT benchmarks and the
number of each instruction matches the previous version I tested.  Can I check
this into the GCC trunk?

2019-11-26  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.md (movsi_internal): Logically align the
	columns of constraints and attributes to make it easier to add new
	patterns in the middle.  Set the num_insns insn attribute.

Comments

Segher Boessenkool Nov. 27, 2019, 5:34 p.m. UTC | #1
Hi Mike,

On Tue, Nov 26, 2019 at 08:32:12PM -0500, Michael Meissner wrote:
> As we discussed in the V6 patches #1 and #2, before submitting the patches
> adding eI support for movdi and movsi, you prefered that I reformat the patches
> to make them easier in the future to identify the changes.

No, that is not the reason.  It is to make it easier to handle patches
(review, revert, rebase, backport, etc.)  A patch should do one thing.

If you need to (say) rebase a big patch that merely changes layout, that
is not so hard to do (just a bit tedious sometimes).  If there are actual
changes mixed in...  *good luck*!

> This patch changes just the movsi_internal insn.  All of the constraints are in
> the same order as the current sources.  I did add setting "num_insns" to this
> patch, but I left in setting "length".  I found otherwise the Spec benchmarks
> did not generate the same code with the patch.

Yes, "length" is still required everywhere, as I said before.  This is a
big problem.

> 	* config/rs6000/rs6000.md (movsi_internal): Logically align the
> 	columns of constraints and attributes to make it easier to add new
> 	patterns in the middle.  Set the num_insns insn attribute.

	* config/rs6000/rs6000.md (movsi_internal): Reformat.
(and the attribute thing, yes, if you have to do that in this same patch.
Please don't, in the future.)

> --- gcc/config/rs6000/rs6000.md	(revision 278746)
> +++ gcc/config/rs6000/rs6000.md	(working copy)
> @@ -6889,24 +6889,36 @@ (define_split
>  				 UNSPEC_MOVSI_GOT))]
>    "")
>  
> -;;		MR           LA           LWZ          LFIWZX       LXSIWZX
> -;;		STW          STFIWX       STXSIWX      LI           LIS
> -;;		#            XXLOR        XXSPLTIB 0   XXSPLTIB -1  VSPLTISW
> -;;		XXLXOR 0     XXLORC -1    P9 const     MTVSRWZ      MFVSRWZ
> -;;		MF%1         MT%0         NOP
> +;;              MR           LA
> +;;              LWZ          LFIWZX      LXSIWZX
> +;;              STW          STFIWX      STXSIWX
> +;;              LI           LIS         #
> +;;              XXLOR        XXSPLTIB 0  XXSPLTIB -1 VSPLTISW
> +;;              XXLXOR 0     XXLORC -1   P9 const
> +;;              MTVSRWZ      MFVSRWZ
> +;;		MF%1         MT%0        NOP

That has broken whitespace (mixing tabs and spaces).  All eight leading
spaces should be tabs, not just on some lines and not the others.

(Similar later, please fix all).

> +                 "*,         *,
> +                 load,       fpload,     fpload,
> +                 store,      fpstore,    fpstore,
> +                 *,          *,          *,
> +                 veclogical, vecsimple,  vecsimple,  vecsimple,
> +                 veclogical, veclogical, vecsimple,
> +                 mffgpr,     mftgpr,
> +                 *,          *,          *")
>     (set_attr "length"
> -		"*,          *,           *,           *,           *,
> -		 *,          *,           *,           *,           *,
> -		 8,          *,           *,           *,           *,
> -		 *,          *,           8,           *,           *,
> -		 *,          *,           *")
> +                 "*,         *,
> +                 *,          *,           *,
> +                 *,          *,           *,
> +                 *,          *,           8,
> +                 *,          *,           *,          *,
> +                 *,          *,           8,
> +                 *,          *,           *,
> +                 *,          *")

You have 2 and 3 on the last two lines elsewhere.

> +   (set_attr "num_insns"
> +                 "*,          *,
> +                 *,           *,         *,
> +                 *,           *,         *,
> +                 *,           *,         2,
> +                 *,           *,         *,          *,
> +                 *,           *,         2,
> +                 *,           *,
> +                 *,           *,         *")

Different indent on the columns here (that is true elsewhere, too).
It would help a lot if you kept all columns the same width, too.

Okay for trunk with the changelog and the layout fixed.  Thanks!


Segher
Michael Meissner Nov. 28, 2019, 12:13 a.m. UTC | #2
Here is the patch that I committed for movsi_internal:

2019-11-27  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.md (movsi_internal): Reformat.

Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 278781)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -6889,24 +6889,34 @@ (define_split
 				 UNSPEC_MOVSI_GOT))]
   "")
 
-;;		MR           LA           LWZ          LFIWZX       LXSIWZX
-;;		STW          STFIWX       STXSIWX      LI           LIS
-;;		#            XXLOR        XXSPLTIB 0   XXSPLTIB -1  VSPLTISW
-;;		XXLXOR 0     XXLORC -1    P9 const     MTVSRWZ      MFVSRWZ
-;;		MF%1         MT%0         NOP
+;;		MR           LA
+;;		LWZ          LFIWZX      LXSIWZX
+;;		STW          STFIWX      STXSIWX
+;;		LI           LIS         #
+;;		XXLOR        XXSPLTIB 0  XXSPLTIB -1 VSPLTISW
+;;		XXLXOR 0     XXLORC -1   P9 const
+;;		MTVSRWZ      MFVSRWZ
+;;		MF%1         MT%0        NOP
+
 (define_insn "*movsi_internal1"
   [(set (match_operand:SI 0 "nonimmediate_operand"
-		"=r,         r,           r,           d,           v,
-		 m,          Z,           Z,           r,           r,
-		 r,          wa,          wa,          wa,          v,
-		 wa,         v,           v,           wa,          r,
-		 r,          *h,          *h")
+		"=r,         r,
+		r,           d,          v,
+		m,           Z,          Z,
+		r,           r,          r,
+		wa,          wa,         wa,         v,
+		wa,          v,          v,
+		wa,          r,
+		r,           *h,         *h")
 	(match_operand:SI 1 "input_operand"
-		"r,          U,           m,           Z,           Z,
-		 r,          d,           v,           I,           L,
-		 n,          wa,          O,           wM,          wB,
-		 O,          wM,          wS,          r,           wa,
-		 *h,         r,           0"))]
+		"r,          U,
+		m,           Z,          Z,
+		r,           d,          v,
+		I,           L,          n,
+		wa,          O,          wM,         wB,
+		O,           wM,         wS,
+		r,           wa,
+		*h,          r,          0"))]
   "gpc_reg_operand (operands[0], SImode)
    || gpc_reg_operand (operands[1], SImode)"
   "@
@@ -6934,23 +6944,32 @@ (define_insn "*movsi_internal1"
    mt%0 %1
    nop"
   [(set_attr "type"
-		"*,          *,           load,        fpload,      fpload,
-		 store,      fpstore,     fpstore,     *,           *,
-		 *,          veclogical,  vecsimple,   vecsimple,   vecsimple,
-		 veclogical, veclogical,  vecsimple,   mffgpr,      mftgpr,
-		 *,          *,           *")
+		"*,         *,
+		load,       fpload,     fpload,
+		store,      fpstore,    fpstore,
+		*,          *,          *,
+		veclogical, vecsimple,  vecsimple,  vecsimple,
+		veclogical, veclogical, vecsimple,
+		mffgpr,     mftgpr,
+		*,          *,          *")
    (set_attr "length"
-		"*,          *,           *,           *,           *,
-		 *,          *,           *,           *,           *,
-		 8,          *,           *,           *,           *,
-		 *,          *,           8,           *,           *,
-		 *,          *,           *")
+		"*,         *,
+		*,          *,           *,
+		*,          *,           *,
+		*,          *,           8,
+		*,          *,           *,          *,
+		*,          *,           8,
+		*,          *,
+		*,          *,           *")
    (set_attr "isa"
-		"*,          *,           *,           p8v,         p8v,
-		 *,          p8v,         p8v,         *,           *,
-		 *,          p8v,         p9v,         p9v,         p8v,
-		 p9v,        p8v,         p9v,         p8v,         p8v,
-		 *,          *,           *")])
+		"*,          *,
+		*,           p8v,       p8v,
+		*,           p8v,       p8v,
+		*,           *,         *,
+		p8v,         p9v,       p9v,       p8v,
+		p9v,         p8v,       p9v,
+		p8v,         p8v,
+		*,           *,         *")])
 
 ;; Like movsi, but adjust a SF value to be used in a SI context, i.e.
 ;; (set (reg:SI ...) (subreg:SI (reg:SF ...) 0))
diff mbox series

Patch

Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 278746)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -6889,24 +6889,36 @@  (define_split
 				 UNSPEC_MOVSI_GOT))]
   "")
 
-;;		MR           LA           LWZ          LFIWZX       LXSIWZX
-;;		STW          STFIWX       STXSIWX      LI           LIS
-;;		#            XXLOR        XXSPLTIB 0   XXSPLTIB -1  VSPLTISW
-;;		XXLXOR 0     XXLORC -1    P9 const     MTVSRWZ      MFVSRWZ
-;;		MF%1         MT%0         NOP
+;;              MR           LA
+;;              LWZ          LFIWZX      LXSIWZX
+;;              STW          STFIWX      STXSIWX
+;;              LI           LIS         #
+;;              XXLOR        XXSPLTIB 0  XXSPLTIB -1 VSPLTISW
+;;              XXLXOR 0     XXLORC -1   P9 const
+;;              MTVSRWZ      MFVSRWZ
+;;		MF%1         MT%0        NOP
+
 (define_insn "*movsi_internal1"
   [(set (match_operand:SI 0 "nonimmediate_operand"
-		"=r,         r,           r,           d,           v,
-		 m,          Z,           Z,           r,           r,
-		 r,          wa,          wa,          wa,          v,
-		 wa,         v,           v,           wa,          r,
-		 r,          *h,          *h")
+               "=r,          r,
+                r,           d,          v,
+                m,           Z,          Z,
+                r,           r,          r,
+                wa,          wa,         wa,         v,
+                wa,          v,          v,
+                wa,          r,
+		r,           *h,         *h")
+
 	(match_operand:SI 1 "input_operand"
-		"r,          U,           m,           Z,           Z,
-		 r,          d,           v,           I,           L,
-		 n,          wa,          O,           wM,          wB,
-		 O,          wM,          wS,          r,           wa,
-		 *h,         r,           0"))]
+                "r,          U,
+                m,           Z,          Z,
+                r,           d,          v,
+                I,           L,          n,
+                wa,          O,          wM,         wB,
+                O,           wM,         wS,
+                r,           wa,
+                *h,          r,          0"))]
+
   "gpc_reg_operand (operands[0], SImode)
    || gpc_reg_operand (operands[1], SImode)"
   "@
@@ -6934,23 +6946,41 @@  (define_insn "*movsi_internal1"
    mt%0 %1
    nop"
   [(set_attr "type"
-		"*,          *,           load,        fpload,      fpload,
-		 store,      fpstore,     fpstore,     *,           *,
-		 *,          veclogical,  vecsimple,   vecsimple,   vecsimple,
-		 veclogical, veclogical,  vecsimple,   mffgpr,      mftgpr,
-		 *,          *,           *")
+                 "*,         *,
+                 load,       fpload,     fpload,
+                 store,      fpstore,    fpstore,
+                 *,          *,          *,
+                 veclogical, vecsimple,  vecsimple,  vecsimple,
+                 veclogical, veclogical, vecsimple,
+                 mffgpr,     mftgpr,
+                 *,          *,          *")
    (set_attr "length"
-		"*,          *,           *,           *,           *,
-		 *,          *,           *,           *,           *,
-		 8,          *,           *,           *,           *,
-		 *,          *,           8,           *,           *,
-		 *,          *,           *")
+                 "*,         *,
+                 *,          *,           *,
+                 *,          *,           *,
+                 *,          *,           8,
+                 *,          *,           *,          *,
+                 *,          *,           8,
+                 *,          *,           *,
+                 *,          *")
+   (set_attr "num_insns"
+                 "*,          *,
+                 *,           *,         *,
+                 *,           *,         *,
+                 *,           *,         2,
+                 *,           *,         *,          *,
+                 *,           *,         2,
+                 *,           *,
+                 *,           *,         *")
    (set_attr "isa"
-		"*,          *,           *,           p8v,         p8v,
-		 *,          p8v,         p8v,         *,           *,
-		 *,          p8v,         p9v,         p9v,         p8v,
-		 p9v,        p8v,         p9v,         p8v,         p8v,
-		 *,          *,           *")])
+                 "*,          *,
+                 *,           p8v,       p8v,
+                 *,           p8v,       p8v,
+                 *,           *,         *,          p8v,
+                 p9v,         p9v,       p8v,
+                 p9v,         p8v,       p9v,
+                 p8v,         p8v,
+		 *,           *,         *")])
 
 ;; Like movsi, but adjust a SF value to be used in a SI context, i.e.
 ;; (set (reg:SI ...) (subreg:SI (reg:SF ...) 0))