diff mbox

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

Message ID 20151014123601.GA38813@msticlxl57.ims.intel.com
State New
Headers show

Commit Message

Kirill Yukhin Oct. 14, 2015, 12:36 p.m. UTC 
Hello,
On 07 Oct 11:09, Jeff Law wrote:
> On 10/05/2015 07:24 AM, Joseph Myers wrote:
> >On Mon, 5 Oct 2015, Kirill Yukhin wrote:
> >
> >>To enable vectorization of loops w/ calls to math functions it is reasonable
> >>to enable parsing of attribute vector for functions unconditionally and
> >>change GlibC's header file not to use `omp declare simd', but use
> >>__attribute__((vector)) instead.
> >
> >I assume you mean __vector__, for namespace reasons.  Obviously you need
> >appropriate GCC version conditionals in the headers to use the attribute
> >only when supported.  In addition, (a) this attribute doesn't seem to be
> >documented in extend.texi, and you'll need to include documentation in
> >your GCC patch that makes this a generic extension rather than just part
> >of Cilkplus, and (b) you'll need to agree with the x86_64 ABI mailing list
> >an extension of the ABI document (as attached to
> ><https://sourceware.org/glibc/wiki/libmvec>) to cover this attribute, and
> >update the document there.
> I'm not sure why this attribute isn't documented, but clearly that should be
> fixed.
> 
> From the GCC side, I don't see a compelling reason to keep this attribute
> conditional on Cilk+ support.   One could very easily want to use the math
> library's vector entry points independent of OpenMP or Cilk+.
> 
> I thought the ABI for this stuff was consistent across the implementations
> (that was certainly the goal).  So aside from an example of how to use the
> attribute to get calls into the vector math library, I'm not sure what's
> needed.  Essentially the attribute is just another way to ensure we exploit
> the vector library when possible.
> 
> It also seems to me that showing that example on the libmvec page would be
> advisable.

Patch in the bottom introduces new attribute called `simd'.
I've decided to introduce a new one since Cilk's `vector' attribute
generates only one version of SIMD-enabled function [1] (which one -
implementation specific).

This new attribute shouldn't be used along w/ Cilk's `vector'.
If it is used w/ `pragma omp declare simd' - it is ignored.

Bootstrapped. New tests pass.

gcc/
	* c/c-parser.c (c_parser): Add simd_attr_present flag.
	(c_parser_declaration_or_fndef): Call c_parser_declaration_or_fndef
	if simd_attr_present is set.
	(c_finish_omp_declare_simd): Handle simd_attr_present.
	* omp-low.c (pass_omp_simd_clone::gate): If target allows - call
	without additional conditions.
gcc/testsuite/
	* c-c++-common/attr-simd.c: New test.
	* c-c++-common/attr-simd-2.c: Ditto.
	* c-c++-common/attr-simd-3.c: Ditto.

Is it ok for trunk?

Here is a description for new attribute:
simd
	Enables creation of one or more versions that can process multiple arguments using
	SIMD instructions from a single invocation from a SIMD loop. It is ultimately an alias
	to `omp declare simd’ pragma, available w/o additional compiler switches.
	It is prohibited to use the attribute along with Cilk Plus’s `vector’ attribute.
	If the attribute is specified and `pragma omp declare simd’ presents on a decl, then
	the attribute is ignored.

[1] - https://www.cilkplus.org/sites/default/files/open_specifications/Intel_Cilk_plus_lang_spec_1.2.htm#elem.attr

--
Thanks, K

Comments

Joseph Myers Oct. 14, 2015, 1:40 p.m. UTC | #1 
On Wed, 14 Oct 2015, Kirill Yukhin wrote:

> Is it ok for trunk?

This patch has no documentation.  Documentation for new attributes must be 
added to extend.texi.

> Enables creation of one or more versions that can process multiple 
> arguments using SIMD instructions from a single invocation from a SIMD 
> loop. It is ultimately an alias to `omp declare simd’ pragma, available 
> w/o additional compiler switches. It is prohibited to use the attribute 
> along with Cilk Plus’s `vector’ attribute. If the attribute is specified 
> and `pragma omp declare simd’ presents on a decl, then the attribute is 
> ignored.

This is missing the key information that it's not just about *creation* of 
the versions, it's about (in the case of an external declaration) 
*assuming* such versions were created in another translation unit.  And 
you should have a link to the external ABI documents specifying for each 
architecture for which this involves such an assumption exactly what 
versions may be assumed to be present - this is what's required to be able 
to use the attribute in headers for a library and know that future GCC 
versions won't reinterpret the attribute as implying some versions for 
future ISA extensions are also present.

I wonder whether the syntax for this attribute should allow optional 
arguments to describe what versions are present, but maybe that can be 
deferred until e.g. you want a way in future for a library to specify it 
has an AVX1024 version of a function as well as the baseline ABI set of 
versions.
diff mbox

Patch

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 2d24c21..b83c9d8 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -224,6 +224,9 @@  struct GTY(()) c_parser {
   /* Buffer to hold all the tokens from parsing the vector attribute for the
      SIMD-enabled functions (formerly known as elemental functions).  */
   vec <c_token, va_gc> *cilk_simd_fn_tokens;
+
+  /* Designates if "simd" attribute is specified in decl.  */
+  BOOL_BITFIELD simd_attr_present : 1;
 };
 
 
@@ -1700,7 +1703,8 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
       if (declarator == NULL)
 	{
 	  if (omp_declare_simd_clauses.exists ()
-	      || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
+	      || !vec_safe_is_empty (parser->cilk_simd_fn_tokens)
+	      || parser->simd_attr_present)
 	    c_finish_omp_declare_simd (parser, NULL_TREE, NULL_TREE,
 				       omp_declare_simd_clauses);
 	  c_parser_skip_to_end_of_block_or_statement (parser);
@@ -1796,7 +1800,8 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 		  if (!d)
 		    d = error_mark_node;
 		  if (omp_declare_simd_clauses.exists ()
-		      || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
+		      || !vec_safe_is_empty (parser->cilk_simd_fn_tokens)
+		      || parser->simd_attr_present)
 		    c_finish_omp_declare_simd (parser, d, NULL_TREE,
 					       omp_declare_simd_clauses);
 		}
@@ -1809,7 +1814,8 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 		  if (!d)
 		    d = error_mark_node;
 		  if (omp_declare_simd_clauses.exists ()
-		      || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
+		      || !vec_safe_is_empty (parser->cilk_simd_fn_tokens)
+		      || parser->simd_attr_present)
 		    c_finish_omp_declare_simd (parser, d, NULL_TREE,
 					       omp_declare_simd_clauses);
 		  start_init (d, asm_name, global_bindings_p ());
@@ -1838,7 +1844,8 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 				   chainon (postfix_attrs,
 					    all_prefix_attrs));
 	      if (omp_declare_simd_clauses.exists ()
-		  || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
+		  || !vec_safe_is_empty (parser->cilk_simd_fn_tokens)
+		  || parser->simd_attr_present)
 		{
 		  tree parms = NULL_TREE;
 		  if (d && TREE_CODE (d) == FUNCTION_DECL)
@@ -1967,7 +1974,8 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 				       true, false, NULL, vNULL);
       store_parm_decls ();
       if (omp_declare_simd_clauses.exists ()
-	  || !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
+	  || !vec_safe_is_empty (parser->cilk_simd_fn_tokens)
+	  || parser->simd_attr_present)
 	c_finish_omp_declare_simd (parser, current_function_decl, NULL_TREE,
 				   omp_declare_simd_clauses);
       DECL_STRUCT_FUNCTION (current_function_decl)->function_start_locus
@@ -3998,6 +4006,12 @@  c_parser_attributes (c_parser *parser)
 	      c_parser_cilk_simd_fn_vector_attrs (parser, *v_token);
 	      continue;
 	    }
+	  if (is_attribute_p ("simd", attr_name))
+	    {
+	      parser->simd_attr_present = 1;
+	      c_parser_consume_token (parser);
+	      continue;
+	    }
 	  c_parser_consume_token (parser);
 	  if (c_parser_next_token_is_not (parser, CPP_OPEN_PAREN))
 	    {
@@ -14210,9 +14224,10 @@  c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
 			   vec<c_token> clauses)
 {
   if (flag_cilkplus
-      && clauses.exists () && !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
+      && (clauses.exists () || parser->simd_attr_present)
+      && !vec_safe_is_empty (parser->cilk_simd_fn_tokens))
     {
-      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");
       vec_free (parser->cilk_simd_fn_tokens);
       return;
@@ -14257,41 +14272,63 @@  c_finish_omp_declare_simd (c_parser *parser, tree fndecl, tree parms,
       parser->tokens = clauses.address ();
       parser->tokens_avail = clauses.length ();
     }
-  
-  /* c_parser_omp_declare_simd pushed 2 extra CPP_EOF tokens at the end.  */
-  while (parser->tokens_avail > 3)
+
+  if (parser->simd_attr_present
+      && is_cilkplus_cilk_simd_fn)
     {
-      c_token *token = c_parser_peek_token (parser);
-      if (!is_cilkplus_cilk_simd_fn) 
-	gcc_assert (token->type == CPP_NAME 
-		    && strcmp (IDENTIFIER_POINTER (token->value), "simd") == 0);
-      else
-	gcc_assert (token->type == CPP_NAME
-		    && is_cilkplus_vector_p (token->value));
-      c_parser_consume_token (parser);
-      parser->in_pragma = true;
+      error ("SIMD-enabled function attributes"
+	     "are allowed when attribute __simd__ is specified");
+      clauses[0].type = CPP_EOF;
+      return;
+    }
 
+  /* Attach `omp declare simd’ attribute if __simd__ is specified AND no OpenMP clauses
+     present in decl.  */
+  if (parser->simd_attr_present
+      && parser->tokens_avail == 0)
+    {
       tree c = NULL_TREE;
-      if (is_cilkplus_cilk_simd_fn) 
-	c = c_parser_omp_all_clauses (parser, CILK_SIMD_FN_CLAUSE_MASK,
-				      "SIMD-enabled functions attribute");
-      else 
-	c = c_parser_omp_all_clauses (parser, OMP_DECLARE_SIMD_CLAUSE_MASK,
-				      "#pragma omp declare simd");
-      c = c_omp_declare_simd_clauses_to_numbers (parms, c);
-      if (c != NULL_TREE)
-	c = tree_cons (NULL_TREE, c, NULL_TREE);
-      if (is_cilkplus_cilk_simd_fn) 
-	{ 
-	  tree k = build_tree_list (get_identifier ("cilk simd function"), 
-				    NULL_TREE);
-	  TREE_CHAIN (k) = DECL_ATTRIBUTES (fndecl);
-	  DECL_ATTRIBUTES (fndecl) = k;
-	} 
       c = build_tree_list (get_identifier ("omp declare simd"), c);
       TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
       DECL_ATTRIBUTES (fndecl) = c;
     }
+  else
+    {
+      /* c_parser_omp_declare_simd pushed 2 extra CPP_EOF tokens at the end.  */
+      while (parser->tokens_avail > 3)
+	{
+	  c_token *token = c_parser_peek_token (parser);
+	  if (!is_cilkplus_cilk_simd_fn)
+	    gcc_assert (token->type == CPP_NAME
+			&& strcmp (IDENTIFIER_POINTER (token->value), "simd") == 0);
+	  else
+	    gcc_assert (token->type == CPP_NAME
+			&& is_cilkplus_vector_p (token->value));
+	  c_parser_consume_token (parser);
+	  parser->in_pragma = true;
+
+	  tree c = NULL_TREE;
+	  if (is_cilkplus_cilk_simd_fn)
+	    c = c_parser_omp_all_clauses (parser, CILK_SIMD_FN_CLAUSE_MASK,
+					  "SIMD-enabled functions attribute");
+	  else
+	    c = c_parser_omp_all_clauses (parser, OMP_DECLARE_SIMD_CLAUSE_MASK,
+					  "#pragma omp declare simd");
+	  c = c_omp_declare_simd_clauses_to_numbers (parms, c);
+	  if (c != NULL_TREE)
+	    c = tree_cons (NULL_TREE, c, NULL_TREE);
+	  if (is_cilkplus_cilk_simd_fn)
+	    {
+	      tree k = build_tree_list (get_identifier ("cilk simd function"),
+					NULL_TREE);
+	      TREE_CHAIN (k) = DECL_ATTRIBUTES (fndecl);
+	      DECL_ATTRIBUTES (fndecl) = k;
+	    }
+	  c = build_tree_list (get_identifier ("omp declare simd"), c);
+	  TREE_CHAIN (c) = DECL_ATTRIBUTES (fndecl);
+	  DECL_ATTRIBUTES (fndecl) = c;
+	}
+    }
 
   parser->tokens = &parser->tokens_buf[0];
   parser->tokens_avail = tokens_avail;
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index cdcf9d6..87537cd 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -13919,10 +13919,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..263fa9a
--- /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 "simdclone" "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..5dad89c
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/attr-simd.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-fdump-tree-optimized" } */
+
+__attribute__((__simd__))
+static int simd_attr (void)
+{
+  return 0;
+}
+
+/* { dg-final { scan-tree-dump "simdclone" "optimized" } } */