Re: [PATCH], Add power9 support to GCC, patch #5 (ISA 3.0 fusion)

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

On Mon, Nov 09, 2015 at 11:16:27AM -0600, Segher Boessenkool wrote:
> On Sun, Nov 08, 2015 at 07:42:04PM -0500, Michael Meissner wrote:
> > -  /* Power8 currently will only do the fusion if the top 11 bits of the addis
> > -     value are all 1's or 0's.  */
> >    value = INTVAL (int_const);
> >    if ((value & (HOST_WIDE_INT)0xffff) != 0)
> 
> Space after cast, like  (HOST_WIDE_INT) 0xffff  .

Thanks.

> > +  /* Power8 currently will only do the fusion if the top 11 bits of the addis
> > +     value are all 1's or 0's.  Ignore this restriction if we are testing
> > +     advanced fusion.  */
> > +  if (TARGET_P9_FUSION)
> > +    return 1;
> 
> This comment seems out of date?

Yeah, when I first coded it when the fusion semantics were being nailed down, I
couldn't reference power9 in the branch which was kept on the FSF servers, so I
just called it advanced fusion.  I evidently missed a few places in doing the
merge to change the name.

> >  ;; Match a GPR load (lbz, lhz, lwz, ld) that uses a combined address in the
> >  ;; memory field with both the addis and the memory offset.  Sign extension
> >  ;; is not handled here, since lha and lwa are not fused.
> > -(define_predicate "fusion_gpr_mem_combo"
> > -  (match_code "mem,zero_extend")
> > +;; With extended fusion, also match a FPR load (lfd, lfs) and float_extend
> 
> And here?

Yes.

> > --- gcc/config/rs6000/rs6000.c	(revision 229975)
> > +++ gcc/config/rs6000/rs6000.c	(working copy)
> > @@ -376,8 +376,18 @@ struct rs6000_reg_addr {
> >    enum insn_code reload_fpr_gpr;	/* INSN to move from FPR to GPR.  */
> >    enum insn_code reload_gpr_vsx;	/* INSN to move from GPR to VSX.  */
> >    enum insn_code reload_vsx_gpr;	/* INSN to move from VSX to GPR.  */
> > +  enum insn_code fusion_gpr_ld;		/* INSN for fusing gpr ADDIS/loads.  */
> > +					/* INSNs for fusing addi with loads
> > +					   or stores for each reg. class.  */					   
> > +  enum insn_code fusion_addi_ld[(int)N_RELOAD_REG];
> > +  enum insn_code fusion_addi_st[(int)N_RELOAD_REG];
> > +					/* INSNs for fusing addis with loads
> > +					   or stores for each reg. class.  */					   
> 
> Trailing tabs.

Ok.

> > +/* Return true if the peephole2 can combine a load/store involving a
> > +   combination of an addis instruction and the memory operation.  This was
> > +   added to the ISA 3.0 (power9) hardware.  */
> > +
> > +bool
> > +fusion_p9_p (rtx addis_reg,		/* register set via addis.  */
> > +	     rtx addis_value,		/* addis value.  */
> > +	     rtx dest,			/* destination (memory or register). */
> > +	     rtx src)			/* source (register or memory).  */
> 
> The function header comment should explain the params, after which you
> can use the normal style for the function declaration itself.

Ok.

> > +(define_insn "*toc_fusionload_<mode>"
> > +  [(set (match_operand:QHSI 0 "int_reg_operand" "=&b,??r")
> > +	(match_operand:QHSI 1 "toc_fusion_mem_wrapped" "wG,wG"))
> > +   (unspec [(const_int 0)] UNSPEC_FUSION_ADDIS)
> > +   (use (match_operand:DI 2 "base_reg_operand" "r,r"))
> > +   (clobber (match_scratch:DI 3 "=X,&b"))]
> > +  "TARGET_TOC_FUSION_INT"
> 
> Do you need that "??r" alternative?  Same for the next define_insn.

Yes unfortunately.  The ??r catches the case where r0 is chosen.  R0 is not a
base register, and it can't be used for power8 gpr fusion (where you use the
value being loaded for the ADDIS instruction), but it can be used for power9
fusion (where the ADDIS must be adjancent, but it no longer has to be the
register being loaded).

> Big patch, most looks good :-)

Thanks.

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