[PATCH], PR 71977/70568/78823: Improve PowerPC code that uses SFmode in unions

[David Edelsohn]

On Thu, Dec 29, 2016 at 7:24 PM, Michael Meissner
<meissner@linux.vnet.ibm.com> wrote:

Mike,

Thanks for the tremendous effort on this patch.  A few comments.

The ChangeLog contains a lot of extraneous commentary that should be
in the documentation comments.  And a few typos in comments.

>         * config/rs6000/predicates.md (sf_subreg_operand): New predicate
>         to return true if the operand contains a SUBREG mixing SImode and
>         SFmode on 64-bit VSX systems with direct move.  This can be a
>         problem in that we have to know whether the SFmode value should be
>         represented in the 32-bit memory format or the 64-bit scalar
>         format used within the floating point and vector registers.

The ChangeLog entry should just say "New predicate."  The description
is in the documentation of the function.

>         * config/rs6000/vsx.md (SFBOOL_*): Add peephole2 to recognize when
>         we are converting a SFmode to a SImode, moving the result to a GPR
>         register, doing a single AND/IOR/XOR operation, and then moving it
>         back to a vector register.  Change the insns recognized to move
>         the integer value to the vector register and do the operation
>         there.  This code occurs quite a bit in the GLIBC math library in
>         float math functions.

SFBOOL constants are not a new peephole2.  Please move the explanation
into a documentation comment in front of the peephole2.

>         (peephole2 to speed up GLIB math functions): Likewise.
                                                   ^^^ GLIBC?
New peephole2.

>         (movsi_from_sf): New insn to handle where we are moving SFmode
>         values to SImode registers and we need to convert the value to the
>         memory format from the format used within the register.

New define_insn_and_split.

>        (movdi_from_sf_zero_ext): Optimize zero extending movsi_from_sf.

New define_insn_and_split.

>         (movsf_from_si): New insn to handle where we are moving SImode
>         values to SFmode registers and we need to convert the value to the
>         the format used within the floating point and vector registers to
>         the 32-bit memory format.

New define_insn_and_split.

+;; Return 1 if op is a SUBREG that is used to look at a SFmode value as
+;; and integer or vice versa.
     ^^ an integer?

+  /*.  Allow (set (SUBREG:SI (REG:SF)) (SUBREG:SI (REG:SF))).  */
      ^ No period, one space.

The change to rs6000_emit_move() really should have been in a helper
function. We have to stop adding to the complexity of the function.  I
won't insist that you split it out, but any addition of more than a
few lines in rs6000_emit_move(), prologue or epilogue should be placed
into a separate function.

Okay with those changes.  This change is too invasive for GCC 6
backport at this late phase of GCC 6 release.

We'll see if Segher catches anything additional when he wants some
light reading. :-)

Thanks, David