i386: Allow setting target attribute from conditional expression

Message ID 20221017144437.157424-1-jwjagersma@gmail.com
State Accepted
Headers
Series i386: Allow setting target attribute from conditional expression |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

J.W. Jagersma Oct. 17, 2022, 2:44 p.m. UTC
  Recently I tried to set a function's target attribute conditionally
based on template parameters, eg.:

    template<bool enable_sse>
    [[gnu::target (enable_sse ? "sse" : "")]]
    void func () { /* ... */ }

I then discovered that this is currently not possible.  This small patch
resolves that.

A possible alternative solution is to do this globally, eg. in
decl_attributes.  But doing so would trigger empty-string warnings from
handle_target_attribute, and I don't know how safe it is to remove that.
There likely isn't much use for this with other attributes, anyway.

2022-10-17  Jan W. Jagersma  <jwjagersma@gmail.com>

gcc/ChangeLog:
	* config/i386/i386-options.cc
	(ix86_valid_target_attribute_inner_p):  Dereference args string
	from ADDR_EXPR.

gcc/testsuite/ChangeLog:
	* g++.target/i386/target-attr-conditional.C: New test.
---
 gcc/config/i386/i386-options.cc               |  9 ++++
 .../g++.target/i386/target-attr-conditional.C | 53 +++++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 gcc/testsuite/g++.target/i386/target-attr-conditional.C
  

Comments

J.W. Jagersma Nov. 8, 2022, 9:15 p.m. UTC | #1
I realize this is a feature with a somewhat niche use-case, but I'd really
like to have it in gcc 13, if possible.  Any feedback is appreciated.

On 2022-10-17 16:44, J.W. Jagersma wrote:
> Recently I tried to set a function's target attribute conditionally
> based on template parameters, eg.:
> 
>     template<bool enable_sse>
>     [[gnu::target (enable_sse ? "sse" : "")]]
>     void func () { /* ... */ }
> 
> I then discovered that this is currently not possible.  This small patch
> resolves that.
> 
> A possible alternative solution is to do this globally, eg. in
> decl_attributes.  But doing so would trigger empty-string warnings from
> handle_target_attribute, and I don't know how safe it is to remove that.
> There likely isn't much use for this with other attributes, anyway.
> 
> 2022-10-17  Jan W. Jagersma  <jwjagersma@gmail.com>
> 
> gcc/ChangeLog:
> 	* config/i386/i386-options.cc
> 	(ix86_valid_target_attribute_inner_p):  Dereference args string
> 	from ADDR_EXPR.
> 
> gcc/testsuite/ChangeLog:
> 	* g++.target/i386/target-attr-conditional.C: New test.
> ---
>  gcc/config/i386/i386-options.cc               |  9 ++++
>  .../g++.target/i386/target-attr-conditional.C | 53 +++++++++++++++++++
>  2 files changed, 62 insertions(+)
>  create mode 100644 gcc/testsuite/g++.target/i386/target-attr-conditional.C
> 
> diff --git a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc
> index acb2291e70f..915f3b0c1f0 100644
> --- a/gcc/config/i386/i386-options.cc
> +++ b/gcc/config/i386/i386-options.cc
> @@ -1123,6 +1123,15 @@ ix86_valid_target_attribute_inner_p (tree fndecl, tree args, char *p_strings[],
>      = fndecl == NULL ? UNKNOWN_LOCATION : DECL_SOURCE_LOCATION (fndecl);
>    const char *attr_name = target_clone_attr ? "target_clone" : "target";
>  
> +  args = tree_strip_nop_conversions (args);
> +
> +  if (TREE_CODE (args) == ADDR_EXPR)
> +    {
> +      /* Attribute string is given by a constexpr function or conditional
> +	 expression.  Dereference ADDR_EXPR, operand should be a STRING_CST.  */
> +      args = TREE_OPERAND (args, 0);
> +    }
> +
>    /* If this is a list, recurse to get the options.  */
>    if (TREE_CODE (args) == TREE_LIST)
>      {
> diff --git a/gcc/testsuite/g++.target/i386/target-attr-conditional.C b/gcc/testsuite/g++.target/i386/target-attr-conditional.C
> new file mode 100644
> index 00000000000..2d418ed90bf
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/i386/target-attr-conditional.C
> @@ -0,0 +1,53 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Wno-psabi -m32 -march=i386 -std=c++20" } */
> +
> +#pragma GCC push_options
> +#pragma GCC target("sse")
> +
> +typedef int __m64 __attribute__ ((__vector_size__ (8), __may_alias__));
> +typedef short __v4hi __attribute__ ((__vector_size__ (8)));
> +
> +extern __inline int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
> +_mm_extract_pi16 (__m64 const __A, int const __N)
> +{
> +  return (unsigned short) __builtin_ia32_vec_ext_v4hi ((__v4hi)__A, __N);
> +}
> +
> +#pragma GCC pop_options
> +
> +consteval const char*
> +target_string (bool enable_sse)
> +{
> +  return enable_sse ? "sse" : "";
> +}
> +
> +// Via consteval function
> +template<bool enable_sse>
> +[[gnu::target (target_string (enable_sse))]]
> +int
> +extract1 (__m64 const src)
> +{
> +  if constexpr (enable_sse)
> +    return _mm_extract_pi16 (src, 0);
> +  else
> +    return reinterpret_cast<__v4hi>(src)[1];
> +}
> +
> +// Via ternary operator
> +template<bool enable_sse>
> +[[gnu::target (enable_sse ? "sse" : "")]]
> +int
> +extract2 (__m64 const src)
> +{
> +  if constexpr (enable_sse)
> +    return _mm_extract_pi16 (src, 2);
> +  else
> +    return reinterpret_cast<__v4hi>(src)[3];
> +}
> +
> +int
> +test (__m64 const src)
> +{
> +  return extract1<true>(src) + extract1<false>(src)
> +       + extract2<true>(src) + extract2<false>(src);
> +}
  

Patch

diff --git a/gcc/config/i386/i386-options.cc b/gcc/config/i386/i386-options.cc
index acb2291e70f..915f3b0c1f0 100644
--- a/gcc/config/i386/i386-options.cc
+++ b/gcc/config/i386/i386-options.cc
@@ -1123,6 +1123,15 @@  ix86_valid_target_attribute_inner_p (tree fndecl, tree args, char *p_strings[],
     = fndecl == NULL ? UNKNOWN_LOCATION : DECL_SOURCE_LOCATION (fndecl);
   const char *attr_name = target_clone_attr ? "target_clone" : "target";
 
+  args = tree_strip_nop_conversions (args);
+
+  if (TREE_CODE (args) == ADDR_EXPR)
+    {
+      /* Attribute string is given by a constexpr function or conditional
+	 expression.  Dereference ADDR_EXPR, operand should be a STRING_CST.  */
+      args = TREE_OPERAND (args, 0);
+    }
+
   /* If this is a list, recurse to get the options.  */
   if (TREE_CODE (args) == TREE_LIST)
     {
diff --git a/gcc/testsuite/g++.target/i386/target-attr-conditional.C b/gcc/testsuite/g++.target/i386/target-attr-conditional.C
new file mode 100644
index 00000000000..2d418ed90bf
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/target-attr-conditional.C
@@ -0,0 +1,53 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wno-psabi -m32 -march=i386 -std=c++20" } */
+
+#pragma GCC push_options
+#pragma GCC target("sse")
+
+typedef int __m64 __attribute__ ((__vector_size__ (8), __may_alias__));
+typedef short __v4hi __attribute__ ((__vector_size__ (8)));
+
+extern __inline int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
+_mm_extract_pi16 (__m64 const __A, int const __N)
+{
+  return (unsigned short) __builtin_ia32_vec_ext_v4hi ((__v4hi)__A, __N);
+}
+
+#pragma GCC pop_options
+
+consteval const char*
+target_string (bool enable_sse)
+{
+  return enable_sse ? "sse" : "";
+}
+
+// Via consteval function
+template<bool enable_sse>
+[[gnu::target (target_string (enable_sse))]]
+int
+extract1 (__m64 const src)
+{
+  if constexpr (enable_sse)
+    return _mm_extract_pi16 (src, 0);
+  else
+    return reinterpret_cast<__v4hi>(src)[1];
+}
+
+// Via ternary operator
+template<bool enable_sse>
+[[gnu::target (enable_sse ? "sse" : "")]]
+int
+extract2 (__m64 const src)
+{
+  if constexpr (enable_sse)
+    return _mm_extract_pi16 (src, 2);
+  else
+    return reinterpret_cast<__v4hi>(src)[3];
+}
+
+int
+test (__m64 const src)
+{
+  return extract1<true>(src) + extract1<false>(src)
+       + extract2<true>(src) + extract2<false>(src);
+}