Steve Ellcey <sell...@marvell.com> writes:
> This is the modified version of the second of my Aarch64 SIMD ABI patches.
> While implementing this functionality I found I wanted
> targetm.simd_clone.adjust to be called when creating SIMD clone definitions
> and also when creating SIMD clone declarations. The current
> implementation (used only by x86) only called this target function when
> creating clone definitions. I added a second argument to the target
> function to say if it was creating a definition or a declaration and
> modified the i386 code to do nothing on declarations, thus maintaining
> its current behavour.
>
> This allowed my to add the aarch64_vector_pcs attribute to SIMD clone
> declarations and definitions on Aarch64.
>
> I considered comparing node->decl and cfun->decl to differentiate
> between definitions and declarations instead of using a new argument
> but having an argument seemed cleaner and clearer.
Yeah, agreed.
> + tree ret_type = TREE_TYPE (TREE_TYPE (node->decl));
> + if (TREE_CODE (ret_type) != VOID_TYPE)
> + switch (TYPE_MODE (ret_type))
> + {
> + case E_QImode:
> + case E_HImode:
> + case E_SImode:
> + case E_DImode:
> + case E_SFmode:
> + case E_DFmode:
> + /* case E_SCmode: */
> + /* case E_DCmode: */
> + break;
> + default:
> + warning_at (DECL_SOURCE_LOCATION (node->decl), 0,
> + "unsupported return type %qT for simd\n", ret_type);
> + return 0;
> + }
I think this should be using the tree-level properties of the type
rather than the mode. E.g.:
struct S { int i[1]; };
has SImode (I think) but the vector ABI's "PBV" property is false for that.
I think we then need to separate cases in which the code is warning
about things that are invalid according to the vector ABI vs. those
that GCC simply doesn't support yet. At the moment I think there's
an implicit requirement in the vector ABI that PBV has to be true for
non-void return types, so warning about the struct above would be a
correctness test.
But as the commented-out lines say, complex types are also valid,
so if we don't support those yet, I think the warning should say
something like "GCC does not yet support ...". ("sorry (...)" is
too strong, since it's error-class.)
We should handle 16-bit floats as well.
> + tree t;
> + for (t = DECL_ARGUMENTS (node->decl); t; t = DECL_CHAIN (t))
> + /* FIXME: Shouldn't we allow such arguments if they are uniform? */
Yeah.
> + switch (TYPE_MODE (TREE_TYPE (t)))
> + {
> + case E_QImode:
> + case E_HImode:
> + case E_SImode:
> + case E_DImode:
> + case E_SFmode:
> + case E_DFmode:
> + /* case E_SCmode: */
> + /* case E_DCmode: */
> + break;
> + default:
> + warning_at (DECL_SOURCE_LOCATION (node->decl), 0,
> + "unsupported argument type %qT for simd\n", TREE_TYPE (t));
> + return 0;
> + }
Same comments here. The vector ABI doesn't place this restriction on
parameters: if PBV is false, the elements are passed by pointer instead.
> +
> + if (TARGET_SIMD)
> + {
> + clonei->vecsize_mangle = 'n';
> + clonei->mask_mode = VOIDmode;
> + clonei->vecsize_int = 128;
> + clonei->vecsize_float = 128;
> +
> + if (clonei->simdlen == 0)
> + {
> + if (SCALAR_INT_MODE_P (TYPE_MODE (base_type)))
> + clonei->simdlen = clonei->vecsize_int;
> + else
> + clonei->simdlen = clonei->vecsize_float;
> + clonei->simdlen /= GET_MODE_BITSIZE (SCALAR_TYPE_MODE (base_type));
Are we sure base_type is correct for the AArch64 vector ABI?
I think at the moment it's specific to the Intel ABI.
> + }
> + else if (clonei->simdlen > 16)
> + {
> + /* If it is possible for given SIMDLEN to pass CTYPE value in
> + registers (v0-v7) accept that SIMDLEN, otherwise warn and don't
> + emit corresponding clone. */
This too would be a GCC restriction rather than an ABI one. Could you
add a comment explaning why we need it?
> +/* If SIMD clone NODE can't be used in a vectorized loop
> + in current function, return -1, otherwise return a badness of using it
> + (0 if it is most desirable from vecsize_mangle point of view, 1
> + slightly less desirable, etc.). */
> +
> +static int
> +aarch64_simd_clone_usable (struct cgraph_node *node)
Think this should just say:
/* Implement TARGET_SIMD_CLONE_USABLE. */
instead of duplicating the hook definition. Maybe there's something
AArch64-specific we can add, but doesn't matter if not.
> diff --git a/gcc/target.def b/gcc/target.def
> index 96f37e0..ffc3787 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -1632,8 +1632,10 @@ int, (struct cgraph_node *, struct cgraph_simd_clone
> *, tree, int), NULL)
> DEFHOOK
> (adjust,
> "This hook should add implicit @code{attribute(target(\"...\"))} attribute\n\
> -to SIMD clone @var{node} if needed.",
> -void, (struct cgraph_node *), NULL)
> +to SIMD clone @var{node} if needed. If the @var{defn} bool argument is
> true\n\
> +then this function is being called for a function definition, if false it
> is\n\
> +a function declaration.",
> +void, (struct cgraph_node *, bool), NULL)
Maybe "this hook is being called", to avoid confusion with the function
argument. It's a pre-existing bug, but the prototype should have names
if the comment refers to them:
void, (struct cgraph_node *node, bool defn), NULL)
Thanks,
Richard
