[PATCH 11/18] rs6000: Builtin expansion, part 6

[Segher Boessenkool]

Hi!

On Wed, Sep 01, 2021 at 11:13:47AM -0500, Bill Schmidt wrote:
> Provide replacements for htm_spr_num and htm_expand_builtin.  No logic
> changes are intended here, as usual.  Much code was factored out into
> rs6000_expand_new_builtin, so the new version of htm_expand_builtin is
> a little tidier.

Nice.

> Also implement the support for the "endian" and "32bit" attributes,
> which is straightforward.  These just do icode substitution.

Don't call this "attributes" please?  I don't know what would be a
better name, of course.  "bif attribute" maybe?

> +  rtx op[MAX_HTM_OPERANDS], pat;

Don't declare arrays and scalars in the same statement, in general.  It
is important that the arrays stand out.

Also, don't declare things before they are used please.

> +  FOR_EACH_CALL_EXPR_ARG (arg, iter, exp)
> +    {
> +      if (arg == error_mark_node || nopnds >= MAX_HTM_OPERANDS)
> +	return const0_rtx;
> +
> +      insn_op = &insn_data[icode].operand[nopnds];
> +      op[nopnds] = expand_normal (arg);
> +
> +      if (!insn_op->predicate (op[nopnds], insn_op->mode))
> +	{
> +	  if (!strcmp (insn_op->constraint, "n"))
> +	    {
> +	      int arg_num = (nonvoid) ? nopnds : nopnds + 1;

Please don't parenthesise random expressions like "nonvoid".  I wonder
if that can be simpler handled by just unshifting a void_node into the
operands, btw :-)

And the same "n" thing as before of course.  Since it is the same: some
factoring would be helpful probably.

> +      machine_mode mode = (TARGET_POWERPC64) ? DImode : SImode;

Superfluous parens.  This is just "word_mode", anyway?

> +  /* If this builtin accesses a CR, then pass in a scratch
> +     CR as the last operand.  */
> +  else if (bif_is_htmcr (*bifaddr))
> +    {
> +      cr = gen_reg_rtx (CCmode);
> +      op[nopnds++] = cr;
> +    }

There is only one CR ("condition register").  You can say CRF here
("condition register field", a 4-bit thing), or just cc or CC maybe
("condition code").  A pet peeve, I know.

> +  if (bif_is_endian (*bifaddr) && BYTES_BIG_ENDIAN)

"is_endian" should maybe be "is_bigendian" or something like that?

Okay for trunk with the changes you see fit at this time.  Thanks!


Segher