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

[Segher Boessenkool <segher at kernel dot crashing dot org>]

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  .

> +  /* 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?

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

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

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

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

Big patch, most looks good :-)


Segher