On Wed, Jan 04, 2017 at 02:17:16PM -0500, David Edelsohn wrote: > On Thu, Dec 29, 2016 at 7:24 PM, Michael Meissner > <meiss...@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: meiss...@linux.vnet.ibm.com, phone: +1 (978) 899-4797
