Re: [PATCH 4/4] [SPARC] Errata workaround for GRLIB-TN-0013

[Eric Botcazou <ebotcazou at adacore dot com>]

> 2017-11-17  Daniel Cederman  <cederman@gaisler.com>
> 
> 	* config/sparc/sparc.c (fpop_reg_depend_p): New function.
> 	(div_sqrt_insn_p): New function.
> 	(sparc_do_work_around_errata): Insert NOP instructions to
> 	prevent sequences that could trigger the TN-0013 errata for
> 	certain LEON3 processors.
> 	(pass_work_around_errata::gate): Also test sparc_fix_tn0013.
> 	(sparc_option_override): Set sparc_fix_tn0013 appropriately.
> 	* config/sparc/sparc.md (fix_tn0013): New attribute.
> 	(in_branch_delay): Prevent div and sqrt in delay slot if fix_tn0013.
> 	* config/sparc/sparc.opt (sparc_fix_tn0013: New variable.

OK for mainline and 7 branch modulo the following nits:

> +/* True if any of the floating-point instruction's source
> +   registers is the same as the provided register.  */
> +
> +static int
> fpop_reg_depend_p (rtx_insn *insn, unsigned int reg)

'bool' instead of 'int'.  "True if any of INSN's source register(s) is REG".

The function's name is awkward.  insn_uses_reg_p?  insn_reads_reg_p?

> +/* True if the instruction is floating-point division or
> +   floating-point square-root.  */
> +
> +static int
> +div_sqrt_insn_p (rtx_insn *insn)

'bool' instead of 'int'. "True if INSN is a floating-point ..."

> +  if ( GET_CODE (PATTERN (insn)) != SET)
> +    return false;

No space before GET_CODE.

> @@ -1064,6 +1097,80 @@ sparc_do_work_around_errata (void)
>  	    insert_nop = true;
>  	}
> 
> +      /* Look for sequences that could trigger the TN-0013 errata.  */
> +      if (sparc_fix_tn0013
> +	  && NONJUMP_INSN_P (insn)
> +	  && div_sqrt_insn_p (insn))
> +	{
> +	  int i;
> +	  int fp_found = 0;
> +	  unsigned int dest_reg;
> +	  rtx_insn *after;
> +
> +	  dest_reg = REGNO (SET_DEST (single_set (insn)));

	  const unsigned int dest_reg = REGNO (SET_DEST (single_set (insn)));

> +	  next = next_active_insn (insn);
> +	  if (!next)
> +	    break;
> +
> +	  for (after = next, i = 0; i < 4; i++)
> +	    {
> +	      /* Count floating-point operations.  */
> +	      if (i != 3
> +		  && fpop_insn_p (after))

On a single line.

> +		  /* If the insn uses the destination register of
> +		     the div/sqrt, then it cannot be problematic.  */
> +		  if (fpop_reg_depend_p (after, dest_reg))
> +		    break;
> +		  fp_found++;
> +		}
> +
> +	      /* Count floating-point loads.  */
> +	      if (i != 3
> +		  && (set = single_set (after)) != NULL_RTX
> +		  && REG_P (SET_DEST (set))
> +		  && REGNO (SET_DEST (set)) > 31)
> +		{
> +		  /* If the insn uses the destination register of
> +		     the div/sqrt, then it cannot be problematic.  */
> +		  if (REGNO (SET_DEST (set)) == dest_reg)
> +		    break;
> +		  fp_found++;
> +		}
> +
> +	      /* Check if this is a problematic sequence.  */
> +	      if (i > 1
> +		  && fp_found >= 2
> +		  && div_sqrt_insn_p (after))
> +		{
> +		  /* Add extra NOP to prevent second version of
> +		     problematic sequence.  */
> +		  if (i == 2)
> +		    emit_insn_before (gen_nop (), next);
> +		  insert_nop = true;
> +		  break;
> +		}

So you emit 2 NOPs in a row if i == 2?

> diff --git a/gcc/config/sparc/sparc.opt b/gcc/config/sparc/sparc.opt
> index 22267f5..43cd964 100644
> --- a/gcc/config/sparc/sparc.opt
> +++ b/gcc/config/sparc/sparc.opt
> @@ -253,6 +253,10 @@ Enable workarounds for the errata of the GR712RC
> processor. TargetVariable
>  unsigned int sparc_fix_b2bst
> 
> +;; Enable workaround for TN-0013 errata
> +TargetVariable
> +unsigned int sparc_fix_tn0013

Can we find a more evocative name for the variable?

-- 
Eric Botcazou