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
