Re: [PATCH] Add __builtin_convertvector support (PR c++/85052)

[Martin Sebor <msebor@gmail.com>]

> +/* Build a VEC_CONVERT ifn for __builtin_convertvector builtin.  */

Can you please document the function arguments and explain how they
are used?

> +
> +tree
> +c_build_vec_convert (location_t loc1, tree expr, location_t loc2, tree type,
> +		     bool complain)
> +{
> +  if (error_operand_p (type))
> +    return error_mark_node;
> +  if (error_operand_p (expr))
> +    return error_mark_node;
> +
> +  if (!VECTOR_INTEGER_TYPE_P (TREE_TYPE (expr))
> +      && !VECTOR_FLOAT_TYPE_P (TREE_TYPE (expr)))
> +    {
> +      if (complain)
> +	error_at (loc1, "%<__builtin_convertvector%> first argument must "
> +			"be an integer or floating vector");
> +      return error_mark_node;
> +    }
> +
> +  if (!VECTOR_INTEGER_TYPE_P (type) && !VECTOR_FLOAT_TYPE_P (type))
> +    {
> +      if (complain)
> +	error_at (loc2, "%<__builtin_convertvector%> second argument must "
> +			"be an integer or floating vector type");
> +      return error_mark_node;
> +    }
> +
> +  if (maybe_ne (TYPE_VECTOR_SUBPARTS (TREE_TYPE (expr)),
> +		TYPE_VECTOR_SUBPARTS (type)))
> +    {
> +      if (complain)
> +	error_at (loc1, "%<__builtin_convertvector%> number of elements "
> +			"of the first argument vector and the second argument "
> +			"vector type should be the same");
> +      return error_mark_node;
> +    }

Just a few wording suggestions for the errors:

1) for the first two errors consider using a single message
    parameterized on the argument number to reduce translation effort
    (both styles are in use but the more concise form seems preferable
    to me)
2) in the last error use "must" instead of "should" as in the first
    two ("must" is imperative rather than just suggestive)
3) consider simplifying the third message to "%<...%> argument
    vectors must have the same size" (or "the same number of
    elements") along the same lines as in c_build_vec_perm_expr.

> +
> +  if ((TYPE_MAIN_VARIANT (TREE_TYPE (TREE_TYPE (expr)))
> +       == TYPE_MAIN_VARIANT (TREE_TYPE (type)))
> +      || (VECTOR_INTEGER_TYPE_P (TREE_TYPE (expr))
> +	  && VECTOR_INTEGER_TYPE_P (type)
> +	  && (TYPE_PRECISION (TREE_TYPE (TREE_TYPE (expr)))
> +	      == TYPE_PRECISION (TREE_TYPE (type)))))
> +    return build1_loc (loc1, VIEW_CONVERT_EXPR, type, expr);

The conditional above is very difficult to read, and without
a comment explaining its purpose, difficult to understand.
Introducing named temporaries for the repetitive subexpressions
would help with the readability (not just here but in rest of
the function as well).  A comment explaining what the conditional
handles would help with the latter.

> +
> +  bool wrap = true;
> +  bool maybe_const = false;
> +  tree ret;

Moving maybe_const to the conditional block where it's used and ret
to the point of its initialization just after that block would improve
readability.

> +  if (!c_dialect_cxx ())
> +    {
> +      /* Avoid C_MAYBE_CONST_EXPRs inside of VEC_CONVERT argument.  */
> +      expr = c_fully_fold (expr, false, &maybe_const);
> +      wrap &= maybe_const;
> +    }
> +
> +  ret = build_call_expr_internal_loc (loc1, IFN_VEC_CONVERT, type, 1, expr);
> +
> +  if (!wrap)
> +    ret = c_wrap_maybe_const (ret, true);
> +
> +  return ret;
>   }

Martin