Re: [PATCH, rs6000] (v2) enable early debug and disable switch for gimple folding

[Segher Boessenkool <segher@kernel.crashing.org>]

On Wed, Aug 09, 2017 at 09:39:08AM -0500, Will Schmidt wrote:
> I also fixed the (missing) space after
> cast for the existing debug code.

Thanks for that :-)


> 	* config/rs6000/rs6000.c: (rs6000_option_override_internal) Add blurb
> 	to indicate when early gimple folding has been disabled.

The colon goes after the closing parenthesis.

> 	(rs6000_invalid_builtin): whitespace.

Capital W.  Or, say what this does?  "Fix whitespace."

> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 1fb9861..f970f9e 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -4180,10 +4180,14 @@ rs6000_option_override_internal (bool global_init_p)
>      {
>        warning (0, N_("-maltivec=le not allowed for big-endian targets"));
>        rs6000_altivec_element_order = 0;
>      }
>  
> +  if (!rs6000_fold_gimple)
> +     fprintf (stderr,
> +     "gimple folding of rs6000 builtins has been disabled.\n");

That first " should line up with the s in stderr.

Should this print a message though?

> +  size_t uns_fncode = (size_t) fn_code;
> +  enum insn_code icode = rs6000_builtin_info[uns_fncode].icode;
> +  const char *fn_name1 = rs6000_builtin_info[uns_fncode].name;
> +  const char *fn_name2 = ((icode != CODE_FOR_nothing)
> +			  ? get_insn_name ((int) icode)
> +			  : "nothing");

Similarly, the ? and : should line up with the second (.
(You can also remove the superfluous outer parens).

> +  if (TARGET_DEBUG_BUILTIN)
> +  {
> +      fprintf (stderr, "rs6000_gimple_fold_builtin %d %s %s\n",
> +	       fn_code, fn_name1, fn_name2);
> +  }

Wrong indent on the {}; but you don't need those here anyway, it's a
single statement.

>      default:
> +	if (TARGET_DEBUG_BUILTIN)
> +	  fprintf (stderr, "gimple builtin intrinsic not matched:%d %s %s \n",
> +			    fn_code, fn_name1, fn_name2);

No space before \n please.

> +mfold-gimple
> +Target Report Var(rs6000_fold_gimple, 1) Init(1)
> +Enable early gimple folding of builtins.

You can drop the ", 1" I think?

Okay with those nits fixed.


Segher