This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, VECTOR ABI] Add __attribute__((__simd__)) to GCC.


On Tue, Nov 10, 2015 at 11:44:18AM +0300, Kirill Yukhin wrote:
> gcc/
>         * omp-low.c (pass_omp_simd_clone::gate): If target allows - call
>         without additional conditions.

Please make sure CHangeLog entries are tab indented.  I would just use
a comma instead of " -" above.

>         * c-c++-common/attr-simd.c: New test.
>         * c-c++-common/attr-simd-2.c: Ditto.
>         * c-c++-common/attr-simd-3.c: Ditto.

New test is IMHO short enough that it is better to spell it
in each case.
>  					       bool *);
> +static tree handle_simd_attribute (tree *, tree, tree, int, bool *);
>  static tree handle_omp_declare_target_attribute (tree *, tree, tree, int,
>  						 bool *);
>  static tree handle_designated_init_attribute (tree *, tree, tree, int, bool *);
> @@ -818,6 +819,8 @@ const struct attribute_spec c_common_attribute_table[] =
>  			      handle_omp_declare_simd_attribute, false },
>    { "cilk simd function",     0, -1, true,  false, false,
>  			      handle_omp_declare_simd_attribute, false },
> +  { "simd",		      0, -1, true,  false, false,
> +			      handle_simd_attribute, false },

Why the -1 in there?  I thought the simd attribute has exactly zero
arguments, so 0, 0.

> +static tree
> +handle_simd_attribute (tree *node, tree name, tree ARG_UNUSED (args),
> +		       int ARG_UNUSED (flags), bool *no_add_attrs)

As per recent discussion, please leave ARG_UNUSED (args) etc. out, just
use unnamed arguments.
> +{
> +  if (TREE_CODE (*node) == FUNCTION_DECL)
> +    {
> +      if (lookup_attribute ("cilk simd function", DECL_ATTRIBUTES (*node)) != NULL)

Too long line.
> +	{
> +	  error_at (DECL_SOURCE_LOCATION (*node),
> +		    "%<__simd__%> attribute cannot be "
> +		    "used in the same function marked as a Cilk Plus SIMD-enabled function");

Too long line.  You should just move "use in the same " one line above.

> +	  *no_add_attrs = true;
> +	}
> +      else
> +	{
> +	  DECL_ATTRIBUTES (*node)
> +	    = tree_cons (get_identifier ("omp declare simd"),
> +			 NULL_TREE, DECL_ATTRIBUTES (*node));	  
> +	}

Please avoid {}s around single statement in the body.
>      {
> -      error ("%<#pragma omp declare simd%> cannot be used in the same "
> +      error ("%<#pragma omp declare simd%> or %<simd%> attribute cannot be used in the same "
>  	     "function marked as a Cilk Plus SIMD-enabled function");

Too long line.

> +      if (lookup_attribute ("simd", DECL_ATTRIBUTES (fndecl)) != NULL)
> +	{
> +	  error_at (DECL_SOURCE_LOCATION (fndecl),
> +		    "%<__simd__%> attribute cannot be "
> +		    "used in the same function marked as a Cilk Plus SIMD-enabled function");

Too long line, see above.

>        c = build_tree_list (get_identifier ("omp declare simd"), c);
>        TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
>        DECL_ATTRIBUTES (fndecl) = c;
> diff --git a/gcc/cp/parser.c b/gcc/cp/parser.c
> index 7555bf3..f3831b9 100644
> --- a/gcc/cp/parser.c
> +++ b/gcc/cp/parser.c
> @@ -34534,10 +34534,11 @@ cp_parser_late_parsing_cilk_simd_fn_info (cp_parser *parser, tree attrs)
>    cp_omp_declare_simd_data *info = parser->cilk_simd_fn_info;
>    int ii = 0;
>  
> -  if (parser->omp_declare_simd != NULL)
> +  if (parser->omp_declare_simd != NULL
> +      || lookup_attribute ("simd", attrs))
>      {
> -      error ("%<#pragma omp declare simd%> cannot be used in the same function"
> -	     " marked as a Cilk Plus SIMD-enabled function");
> +      error ("%<#pragma omp declare simd%> of %<simd%> attribute cannot be used "
> +	     "in the same function marked as a Cilk Plus SIMD-enabled function");

Too long lines.

>        XDELETE (parser->cilk_simd_fn_info);
>        parser->cilk_simd_fn_info = NULL;
>        return attrs;
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index fdb1547..32994a2 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -3066,6 +3066,21 @@ This function attribute make a stack protection of the function if
>  flags @option{fstack-protector} or @option{fstack-protector-strong}
>  or @option{fstack-protector-explicit} are set.
>  
> +@item simd
> +@cindex @code{simd} function attribute.
> +This attribute enables creation of one or more function versions that
> +can process multiple arguments using SIMD instructions from a
> +single invocation.  Specifying this attribute allows compiler to
> +assume that such a versions are available at link time (provided
> +in the same or another translation unit).  Generated versions are
> +target dependent and described in corresponding Vector ABI document.  For
> +x86_64 target this document can be found
> +@w{@uref{https://sourceware.org/glibc/wiki/libmvec?action=AttachFile&do=view&target=VectorABI.txt,here}}.

Are you going to update VectorABI.txt based on the latest changes in
the Intel ABI document?  I mean especially using L, R or U for linear
%val(), %ref() or %uval() (if references), not using s for linear with
uniform parameter stride, but instead using ls, Ls, Rs or Us for those?

> +It is prohibited to use the attribute along with Cilk Plus's @code{vector}
> +attribute. If the attribute is specified and @code{#pragma omp declare simd}
> +presented on a declaration and @code{-fopenmp} or @code{-fopenmp-simd}
> +switch is specified, then the attribute is ignored.
> +
>  @item target (@var{options})
>  @cindex @code{target} function attribute
>  Multiple target back ends implement the @code{target} attribute
> diff --git a/gcc/omp-low.c b/gcc/omp-low.c
> index ad7c017..232dc5c 100644
> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -17412,10 +17412,7 @@ public:
>  bool
>  pass_omp_simd_clone::gate (function *)
>  {
> -  return ((flag_openmp || flag_openmp_simd
> -	   || flag_cilkplus
> -	   || (in_lto_p && !flag_wpa))
> -	  && (targetm.simd_clone.compute_vecsize_and_simdlen != NULL));
> +  return targetm.simd_clone.compute_vecsize_and_simdlen != NULL;
>  }
>  
>  } // anon namespace
> diff --git a/gcc/testsuite/c-c++-common/attr-simd-2.c b/gcc/testsuite/c-c++-common/attr-simd-2.c
> new file mode 100644
> index 0000000..e9afc11
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attr-simd-2.c
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fdump-tree-optimized -fopenmp-simd" } */
> +
> +#pragma omp declare simd
> +__attribute__((__simd__))
> +static int simd_attr (void)
> +{
> +  return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump "omp declare simd" "optimized" } } */
> diff --git a/gcc/testsuite/c-c++-common/attr-simd-3.c b/gcc/testsuite/c-c++-common/attr-simd-3.c
> new file mode 100644
> index 0000000..2bbdf04
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attr-simd-3.c
> @@ -0,0 +1,5 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fcilkplus" } */
> +/* { dg-prune-output "undeclared here \\(not in a function\\)|\[^\n\r\]* was not declared in this scope" } */
> +
> +void f () __attribute__((__simd__, __vector__)); /* { dg-error "in the same function marked as a Cilk Plus" } */
> diff --git a/gcc/testsuite/c-c++-common/attr-simd.c b/gcc/testsuite/c-c++-common/attr-simd.c
> new file mode 100644
> index 0000000..6fd757a
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/attr-simd.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fdump-tree-optimized" } */
> +
> +__attribute__((__simd__))
> +static int simd_attr (void)
> +{
> +  return 0;
> +}
> +
> +__attribute__((simd))
> +static int simd_attr2 (void)
> +{
> +  return 0;
> +}
> +
> +/* { dg-final { scan-tree-dump "simd_attr\[ \\t\]simdclone|vector" "optimized" } } */
> +/* { dg-final { scan-tree-dump "simd_attr2\[ \\t\]simdclone|vector" "optimized" } } */

You should add a few scan-assembler lines for x86_64/i?86 (see some
declare-simd*.{c,C} tests) to test mangling.  Of course not when the
function is static...

Otherwise LGTM.

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]