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

[Eric Botcazou <ebotcazou@adacore.com>]

> 2017-11-17  Daniel Cederman  <cederman@gaisler.com>
> 
> 	* config/sparc/sparc.c (fpop_insn_p): New function.
> 	(sparc_do_work_around_errata): Insert NOP instructions to
> 	prevent sequences that could trigger the TN-0012 errata for
> 	GR712RC.
> 	(pass_work_around_errata::gate): Also test sparc_fix_gr712rc.
> 	* config/sparc/sparc.md (fix_gr712rc): New attribute.
> 	(in_branch_annul_delay): Prevent floating-point instructions
> 	in delay slot of annulled integer branch.

OK for mainline and 7 branch modulo the following nits:

> +/* True if floating-point instruction.  */
> +
> +static int
> +fpop_insn_p (rtx_insn *insn)

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

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

No space before GET_CODE.

>  /* We use a machine specific pass to enable workarounds for errata.
> 
>     We need to have the (essentially) final form of the insn stream in order
> @@ -970,11 +995,31 @@ sparc_do_work_around_errata (void)
>      {
>        bool insert_nop = false;
>        rtx set;
> +      rtx_insn *jump = 0;
> 
>        /* Look into the instruction in a delay slot.  */
>        if (NONJUMP_INSN_P (insn))
>  	if (rtx_sequence *seq = dyn_cast <rtx_sequence *> (PATTERN (insn)))
> -	  insn = seq->insn (1);
> +	  {
> +	    jump = seq->insn (0);
> +	    insn = seq->insn (1);
> +	  }

This should be changed into:

      rtx_insn jump;

      /* Look into the instruction in a delay slot.  */
      if (NONJUMP_INSN_P (insn)
	  && (rtx_sequence *seq = dyn_cast <rtx_sequence *> (PATTERN (insn)))
	{
	  jump = seq->insn (0);
	  insn = seq->insn (1);
	}
      else if (JUMP_P (insn))
	jump = insn
      else
	jump = NULL_RTX;

and the block below simplified accordingly.

> +      /* Place a NOP at the branch target of an integer branch if it is
> +	 a floating-point operation or a floating-point branch.  */
> +      if (sparc_fix_gr712rc
> +	  && (JUMP_P (insn) || jump)
> +	  && get_attr_branch_type (jump ? jump : insn) == BRANCH_TYPE_ICC)
> +	{
> +	  rtx_insn *target;
> +
> +	  target = next_active_insn (JUMP_LABEL_AS_INSN (jump ? jump : insn));

	  rtx_insn *target = next_active_insn (JUMP_LABEL_AS_INSN (jump));

>        /* Look for either of these two sequences:
> 
> @@ -1303,7 +1348,8 @@ public:
>    /* opt_pass methods: */
>    virtual bool gate (function *)
>      {
> -      return sparc_fix_at697f || sparc_fix_ut699 || sparc_fix_b2bst;
> +      return sparc_fix_at697f || sparc_fix_ut699 || sparc_fix_b2bst
> +	|| sparc_fix_gr712rc;
>      }

"|| sparc_fix_gr712rc" lined up under sparc_fix_at697f.

> @@ -602,6 +615,10 @@
>  (define_delay (eq_attr "type" "branch")
>    [(eq_attr "in_branch_delay" "true") (nil) (eq_attr "in_branch_delay"
> "true")])
> 
> +(define_delay (and (eq_attr "type" "branch") (eq_attr "branch_type" "icc"))
> +  [(eq_attr "in_branch_delay" "true") (nil)
> +  (eq_attr "in_branch_annul_delay" "true")])
> +

I think that we'd better keep the various define_delay's mutually exclusive.

-- 
Eric Botcazou