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
