Re: [PATCH] rs6000: Cleanup bdz/bdnz insn/splitter, add new insn/splitter for bdzt/bdzf/bdnzt/bdnzf

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

Hi Aaron,

On Thu, Nov 30, 2017 at 11:31:47AM -0600, Aaron Sawdey wrote:
> This does some cleanup/consolidation so that bdz/bdnz are supported by
> a single insn and splitter, and adds a new insn and splitter to support
> the conditional form of those (bdzt/bdzf/bdnzt/bdnzf).
> 
> This is going to be used for the memcmp() builtin expansion patch which
> is next. That also will require the change to canonicalize_condition I
> posted before thanksgiving to prevent doloop from being confused by
> bdnzt et. al. 

> 	* config/rs6000/rs6000.md (cceq_ior_compare): Remove * so I can use it
> 	to generate rtl.
> 	(cceq_ior_compare_complement): Give it a name so I can use it, and
> 	change boolean_or_operator predicate to boolean_operator so it can
> 	be used to generate a crand.
> 	Define new code iterator eqne, and new code_attrs bd/bd_neg.

	(eqne): New code_iterator.
etc.

> 	(<bd>_<mode>) New name for ctr<mode>_internal[12] now combined into
> 	a single define_insn. There is now just a single splitter for this
> 	that looks whether the decremented ctr result is going to a register
> 	or memory and generates the appropriate rtl.

Colon after ), two spaces after a full stop.  You don't need to explain
what a change is for btw; changelog is *what* changed, not *why*.

> 	(<bd>tf_<mode>) A new insn pattern for the conditional form branch
> 	decrement (bdnzt/bdnzf/bdzt/bdzf). This also has a splitter similar
> 	to the one for <bd>_<mode>.
> 	* config/rs6000/rs6000.c (rs6000_legitimate_combined_insn): Updated
> 	with the new names of the branch decrement patterns, and added the
> 	names of the branch decrement conditional patterns.

> +      && (icode == CODE_FOR_bdz_si
> +	  || icode == CODE_FOR_bdz_di
> +	  || icode == CODE_FOR_bdnz_si
> +	  || icode == CODE_FOR_bdnz_di
> +	  || icode == CODE_FOR_bdztf_si
> +	  || icode == CODE_FOR_bdnztf_si
> +	  || icode == CODE_FOR_bdztf_di
> +	  || icode == CODE_FOR_bdnztf_di))

Please swap bdnztf_si and bdztf_di so it is clearer you handle all cases?

> +(define_code_iterator eqne [eq ne])
> +(define_code_attr bd [(ne "bdnz") (eq "bdz")])
> +(define_code_attr bd_neg [(ne "bdz") (eq "bdnz")])

Maybe order those as eq, ne as well?

> +  rtx ctrin = operands[1];
> +  rtx ctrout = operands[0];
> +  rtx ctrtmp = operands[4];

I don't think these temporaries improve legibility at all?

>    operands[7] = gen_rtx_fmt_ee (GET_CODE (operands[2]), VOIDmode, operands[3],
>  				const0_rtx);
> +  emit_insn (gen_rtx_SET (operands[3],
> +                          gen_rtx_COMPARE (CCmode, ctrin, const1_rtx)));
> +  if (gpc_reg_operand (ctrout, <MODE>mode))
> +    emit_insn (gen_add<mode>3 (ctrout, ctrin, constm1_rtx));
> +  else
> +    {
> +      emit_insn (gen_add<mode>3 (ctrtmp, ctrin, constm1_rtx));
> +      emit_move_insn (ctrout, ctrtmp);
> +    } 

(Space at end of line).

> +    /* No DONE so branch comes from the pattern.  */
>  })

> +(define_insn "<bd>tf_<mode>"
>    [(set (pc)
> -	(if_then_else (match_operator 2 "comparison_operator"
> -				      [(match_operand:P 1 "gpc_reg_operand")
> -				       (const_int 1)])
> -		      (match_operand 5)
> -		      (match_operand 6)))
> -   (set (match_operand:P 0 "nonimmediate_operand")
> +	(if_then_else
> +	  (and
> +	     (eqne (match_operand:P 1 "register_operand" "c,*b,*b,*b")
> +		   (const_int 1))
> +	     (match_operator 3 "branch_comparison_operator"
> +		      [(match_operand 4 "cc_reg_operand" "y,y,y,y")
> +		       (const_int 0)]))
> +		      (label_ref (match_operand 0))
> +		      (pc)))

Those last two lines should be indented the same as the "(and" (they are
sub rtx of the if_then_else).

> +{
> +  if (which_alternative != 0)
> +    return "#";
> +  else if (get_attr_length (insn) == 4)
> +    {
> +      if (branch_positive_comparison_operator (operands[3],
> +                                               GET_MODE (operands[3])))
> +        return "<bd>t %j3,%l0";
> +      else
> +        return "<bd>f %j3,%l0";

Eight leading spaces should be a tab.

> +    }
> +  else
> +    { 

Trailing space.

> +        static char seq[96];
> +        char *bcs = output_cbranch (operands[3], "$+8", 1, insn);
> +	sprintf(seq, "<bd_neg> $+12\;%s;b %%l0", bcs);
> +	return seq;

The indent should be six spaces on these four lines.

It should be "const char *" really; but output_cbranch has a big bug
as well it seems: it returns a pointer to a string on its stack!  Uh-oh.

> +;; Now the splitter if we could not allocate the CTR register
> +(define_split
> +  [(set (pc)
> +	(if_then_else
> +	  (and
> +	     (match_operator 1 "comparison_operator"
> +	                     [(match_operand:P 0 "gpc_reg_operand")
> +		              (const_int 1)])
> +	     (match_operator 3 "branch_comparison_operator"
> +		      [(match_operand 2 "cc_reg_operand")
> +		       (const_int 0)]))
> +		      (match_operand 4)
> +		      (match_operand 5)))

Indent of these last two lines.

> +   if (!ispos)
> +      cmpcode = reverse_condition (cmpcode);

Should indent one space less.

> +   // want to generate crand/crandc
> +
> +   emit_insn (gen_rtx_SET (operands[8],
> +                           gen_rtx_COMPARE (CCmode, ctr, const1_rtx)));

Each eight leading spaces should be a tab.

> +   rtx ctrcmpcc = gen_rtx_fmt_ee (cmpcode, SImode, operands[8], const0_rtx);
> +
> +   rtx andexpr = gen_rtx_AND (SImode, ctrcmpcc, cccmp);
> +   if (ispos)
> +      emit_insn (gen_cceq_ior_compare (operands[9], andexpr, ctrcmpcc,
> +                                       operands[8], cccmp, ccin));
> +   else
> +      emit_insn (gen_cceq_ior_compare_complement (operands[9], andexpr, ctrcmpcc,
> +                                                  operands[8], cccmp, ccin));
> +   if (gpc_reg_operand (operands[0], <MODE>mode))
> +      emit_insn (gen_add<mode>3 (ctrout, ctr, constm1_rtx));
> +   else
> +      {
> +       	 emit_insn (gen_add<mode>3 (ctrtmp, ctr, constm1_rtx));

That has a tab after spaces, never correct.

> +         emit_move_insn (ctrout, ctrtmp);
> +      }
> +   rtx cmp = gen_rtx_EQ (CCEQmode, operands[9], const0_rtx);
> +   emit_jump_insn (gen_rtx_SET (pc_rtx,
> +                                gen_rtx_IF_THEN_ELSE (VOIDmode, cmp,
> +						      dst1, dst2)));
> +   DONE;
>  })

Looks good otherwise.  I'll ok it when there is a user (or a testcase).
It shouldn't go in before the canonicalize_condition patch, of course.

Thanks!


Segher