Re: [PATCH 10/18] rs6000: Builtin expansion, part 5

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

Hi!

On Wed, Sep 01, 2021 at 11:13:46AM -0500, Bill Schmidt wrote:
> 	* config/rs6000/rs6000-call.c (new_mma_expand_builtin):
> 	Implement.

Same comment as all previous here :-)

>  new_mma_expand_builtin (tree exp, rtx target, insn_code icode,
>  			rs6000_gen_builtins fcode)
>  {
> +  tree fndecl = TREE_OPERAND (CALL_EXPR_FN (exp), 0);
> +  tree arg;
> +  call_expr_arg_iterator iter;
> +  const struct insn_operand_data *insn_op;
> +  rtx op[MAX_MMA_OPERANDS];
> +  unsigned nopnds = 0;
> +  bool void_func = TREE_TYPE (TREE_TYPE (fndecl)) == void_type_node;
> +  machine_mode tmode = VOIDmode;

All those declarations could be much later, making things easier to read
and understand (and review and check :-) ).

> +  if (!void_func)
> +    {
> +      tmode = insn_data[icode].operand[0].mode;
> +      if (!target
> +	  || GET_MODE (target) != tmode
> +	  || !insn_data[icode].operand[0].predicate (target, tmode))

Inverted logic again.

> +      if (!insn_op->predicate (opnd, insn_op->mode))
> +	{
> +	  if (!strcmp (insn_op->constraint, "n"))

Is looking at the contraint string as text optimal here?  Is it even
correct?  Hrm.  It should have a comment explaining it, at least.

> +    case 5:
> +      /* The ASSEMBLE builtin source operands are reversed in little-endian
> +	 mode, so reorder them.  */
> +      if (fcode == RS6000_BIF_ASSEMBLE_ACC_INTERNAL && !WORDS_BIG_ENDIAN)
> +	{
> +	  std::swap (op[1], op[4]);
> +	  std::swap (op[2], op[3]);
> +	}

Where is this done in the "old" code?  I probably are just looking with
unsuitable body parts for seeing, but :-)

> +  if (!pat)
> +    return NULL_RTX;
> +  emit_insn (pat);
> +
>    return target;

I'd put a blank line before that emit_insn.  For dramatic tension, if
nothing else ;-)  (It reads better imo).

Okay for trunk.  Thanks!


Segher