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