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

[Michael Meissner <meissner at linux dot vnet dot ibm dot com>]

On Wed, Jan 04, 2017 at 02:17:16PM -0500, David Edelsohn wrote:
> 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.

Ok.

> >         * 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.

Ok.

> >         (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.

Whoops.  How did that get there?

> 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.

I will see about moving it out to a helper function.

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

Yes, I figured it was too invasive for GCC 6, but I know some people want this
patch, ASAP.  In any case, I know it wouldn't just go in, since some of the
constraints are related to VSX small integer support, and that did not make GCC
6.

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

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797