Message ID | 20191127013212.GA31715@ibm-toto.the-meissners.org |
---|---|
State | New |
Headers | show |
Series | Reformat PowerPC movsi_internal | expand |
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
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))
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))