[PATCH] C++: target attribute - local decl

[PATCH] C++: target attribute - local decl

[Martin Liška]

We crash when target attribute get_function_versions_dispatcher is called
for a function that is not registered in call graph. This fixes that
by emitting a new error.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/cp/ChangeLog:

	PR c++/99108
	* call.c (get_function_version_dispatcher): Do not parse
	target attribute for a function with a missing declaration.

gcc/testsuite/ChangeLog:

	PR c++/99108
	* g++.target/i386/pr99108.C: New test.
---
  gcc/cp/call.c                           |  8 +++++++-
  gcc/testsuite/g++.target/i386/pr99108.C | 18 ++++++++++++++++++
  2 files changed, 25 insertions(+), 1 deletion(-)
  create mode 100644 gcc/testsuite/g++.target/i386/pr99108.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 186feef6fe3..844853e504e 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8386,8 +8386,14 @@ get_function_version_dispatcher (tree fn)
  	      && DECL_FUNCTION_VERSIONED (fn));
  
    gcc_assert (targetm.get_function_versions_dispatcher);
-  dispatcher_decl = targetm.get_function_versions_dispatcher (fn);
+  if (cgraph_node::get (fn) == NULL)
+    {
+      error_at (DECL_SOURCE_LOCATION (fn), "missing declaration "
+		"for a multiversioned function");
+      return NULL;
+    }
  
+  dispatcher_decl = targetm.get_function_versions_dispatcher (fn);
    if (dispatcher_decl == NULL)
      {
        error_at (input_location, "use of multiversioned function "
diff --git a/gcc/testsuite/g++.target/i386/pr99108.C b/gcc/testsuite/g++.target/i386/pr99108.C
new file mode 100644
index 00000000000..b0c4ffa2688
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/pr99108.C
@@ -0,0 +1,18 @@
+/* PR c++/99108 */
+/* { dg-do compile { target c++20 } } */
+/* { dg-require-ifunc "" }  */
+
+struct A {
+  void foo(auto);
+};
+void A::foo(auto)
+{
+  int f(void) __attribute__((target("default")));
+  int f(void) __attribute__((target("arch=atom"))); /* { dg-error "missing declaration for a multiversioned function" } */
+  int b = f();
+}
+void bar(void)
+{
+  A c;
+  c.foo(7);
+}
-- 
2.30.0

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

On 2/18/21 7:15 AM, Martin Liška wrote:
> We crash when target attribute get_function_versions_dispatcher is called
> for a function that is not registered in call graph. This fixes that
> by emitting a new error.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
> 
> Ready to be installed?
> Thanks,
> Martin
> 
> gcc/cp/ChangeLog:
> 
>      PR c++/99108
>      * call.c (get_function_version_dispatcher): Do not parse
>      target attribute for a function with a missing declaration.
> 
> gcc/testsuite/ChangeLog:
> 
>      PR c++/99108
>      * g++.target/i386/pr99108.C: New test.
> ---
>   gcc/cp/call.c                           |  8 +++++++-
>   gcc/testsuite/g++.target/i386/pr99108.C | 18 ++++++++++++++++++
>   2 files changed, 25 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.target/i386/pr99108.C
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 186feef6fe3..844853e504e 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -8386,8 +8386,14 @@ get_function_version_dispatcher (tree fn)
>             && DECL_FUNCTION_VERSIONED (fn));
> 
>     gcc_assert (targetm.get_function_versions_dispatcher);
> -  dispatcher_decl = targetm.get_function_versions_dispatcher (fn);
> +  if (cgraph_node::get (fn) == NULL)
> +    {
> +      error_at (DECL_SOURCE_LOCATION (fn), "missing declaration "
> +        "for a multiversioned function");

This seems like the wrong message.  The declaration isn't missing, 
you're using its source location for the error.  Do you mean 
"definition"?  But adding definitions earlier in the file doesn't help. 
And having file-scope declarations without definitions is fine.

The problem seems to be with the handling of local decls.  If 
DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to find the 
namespace-scope decl.  But then if there is no preceding namespace-scope 
declaration, the new decl created by push_local_extern_decl_alias 
doesn't have a cgraph node, either.  I guess maybe_function_versions 
also needs to look through DECL_LOCAL_DECL_ALIAS.

> +      return NULL;
> +    }
> 
> +  dispatcher_decl = targetm.get_function_versions_dispatcher (fn);
>     if (dispatcher_decl == NULL)
>       {
>         error_at (input_location, "use of multiversioned function "
> diff --git a/gcc/testsuite/g++.target/i386/pr99108.C 
> b/gcc/testsuite/g++.target/i386/pr99108.C
> new file mode 100644
> index 00000000000..b0c4ffa2688
> --- /dev/null
> +++ b/gcc/testsuite/g++.target/i386/pr99108.C
> @@ -0,0 +1,18 @@
> +/* PR c++/99108 */
> +/* { dg-do compile { target c++20 } } */
> +/* { dg-require-ifunc "" }  */
> +
> +struct A {
> +  void foo(auto);
> +};
> +void A::foo(auto)
> +{
> +  int f(void) __attribute__((target("default")));
> +  int f(void) __attribute__((target("arch=atom"))); /* { dg-error 
> "missing declaration for a multiversioned function" } */
> +  int b = f();
> +}
> +void bar(void)
> +{
> +  A c;
> +  c.foo(7);
> +}

Re: [PATCH] C++: target attribute - local decl

[Martin Liška]

On 2/22/21 11:53 PM, Jason Merrill wrote:
> The problem seems to be with the handling of local decls.  If DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to find the namespace-scope decl.  But then if there is no preceding namespace-scope declaration, the new decl created by push_local_extern_decl_alias doesn't have a cgraph node, either.  I guess maybe_function_versions also needs to look through DECL_LOCAL_DECL_ALIAS.

Ah, I see. Are you sure about the name 'maybe_function_versions'? I can't find it.

Thanks,
Martin

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

On 3/1/21 7:43 AM, Martin Liška wrote:
> On 2/22/21 11:53 PM, Jason Merrill wrote:
>> The problem seems to be with the handling of local decls.  If 
>> DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to find 
>> the namespace-scope decl.  But then if there is no preceding 
>> namespace-scope declaration, the new decl created by 
>> push_local_extern_decl_alias doesn't have a cgraph node, either.  I 
>> guess maybe_function_versions 
>> also needs to look through DECL_LOCAL_DECL_ALIAS.
> 
> Ah, I see. Are you sure about the name 'maybe_function_versions'? I 
> can't find it.

Ah, it's maybe_version_functions, sorry.

Jason

Re: [PATCH] C++: target attribute - local decl

[Martin Liška]

On 3/1/21 5:36 PM, Jason Merrill wrote:
> On 3/1/21 7:43 AM, Martin Liška wrote:
>> On 2/22/21 11:53 PM, Jason Merrill wrote:
>>> The problem seems to be with the handling of local decls.  If DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to find the namespace-scope decl.  But then if there is no preceding namespace-scope declaration, the new decl created by push_local_extern_decl_alias doesn't have a cgraph node, either.  I guess maybe_function_versions also needs to look through DECL_LOCAL_DECL_ALIAS.
>>
>> Ah, I see. Are you sure about the name 'maybe_function_versions'? I can't find it.
> 
> Ah, it's maybe_version_functions, sorry.

Thanks, I see the function now.
So about your guess:

> I guess maybe_function_versions also needs to look through DECL_LOCAL_DECL_ALIAS.

Do you mean maybe_version_functions's argument 'record' should depend on DECL_LOCAL_DECL_ALIAS of newdecl/oldddecl
(if present)? Or that DECL_FUNCTION_VERSIONED should be set for DECL_LOCAL_DECL_ALIASes of the newdecl/olddecl
function declarations?

Thanks,
Martin

> 
> Jason
> 

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

On 3/1/21 11:59 AM, Martin Liška wrote:
> On 3/1/21 5:36 PM, Jason Merrill wrote:
>> On 3/1/21 7:43 AM, Martin Liška wrote:
>>> On 2/22/21 11:53 PM, Jason Merrill wrote:
>>>> The problem seems to be with the handling of local decls.  If 
>>>> DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to find 
>>>> the namespace-scope decl.  But then if there is no preceding 
>>>> namespace-scope declaration, the new decl created by 
>>>> push_local_extern_decl_alias doesn't have a cgraph node, either.  I 
>>>> guess maybe_function_versions 
>>>> also needs to look through DECL_LOCAL_DECL_ALIAS.
>>>
>>> Ah, I see. Are you sure about the name 'maybe_function_versions'? I 
>>> can't find it.
>>
>> Ah, it's maybe_version_functions, sorry.
> 
> Thanks, I see the function now.
> So about your guess:
> 
>> I guess maybe_function_versions also needs to look through 
>> DECL_LOCAL_DECL_ALIAS.
> 
> Do you mean maybe_version_functions's argument 'record' should depend on 
> DECL_LOCAL_DECL_ALIAS of newdecl/oldddecl
> (if present)? Or that DECL_FUNCTION_VERSIONED should be set for 
> DECL_LOCAL_DECL_ALIASes of the newdecl/olddecl
> function declarations?

The latter.

Jason

Re: [PATCH] C++: target attribute - local decl

[Martin Liška]

On 3/1/21 8:58 PM, Jason Merrill wrote:
> On 3/1/21 11:59 AM, Martin Liška wrote:
>> On 3/1/21 5:36 PM, Jason Merrill wrote:
>>> On 3/1/21 7:43 AM, Martin Liška wrote:
>>>> On 2/22/21 11:53 PM, Jason Merrill wrote:
>>>>> The problem seems to be with the handling of local decls.  If DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to find the namespace-scope decl.  But then if there is no preceding namespace-scope declaration, the new decl created by push_local_extern_decl_alias doesn't have a cgraph node, either.  I guess maybe_function_versions also needs to look through DECL_LOCAL_DECL_ALIAS.
>>>>
>>>> Ah, I see. Are you sure about the name 'maybe_function_versions'? I can't find it.
>>>
>>> Ah, it's maybe_version_functions, sorry.
>>
>> Thanks, I see the function now.
>> So about your guess:
>>
>>> I guess maybe_function_versions also needs to look through DECL_LOCAL_DECL_ALIAS.
>>
>> Do you mean maybe_version_functions's argument 'record' should depend on DECL_LOCAL_DECL_ALIAS of newdecl/oldddecl
>> (if present)? Or that DECL_FUNCTION_VERSIONED should be set for DECL_LOCAL_DECL_ALIASes of the newdecl/olddecl
>> function declarations?
> 
> The latter.

I see, but will not help us. Problem is that
#2  0x00000000015d8899 in ix86_get_function_versions_dispatcher (decl=0x7ffff7755000) at /home/marxin/Programming/gcc/gcc/config/i386/i386-features.c:2862

is called for a declaration for which

Breakpoint 5, maybe_version_functions (newdecl=<function_decl 0x7ffff7755000 f>, olddecl=<function_decl 0x7ffff7751f00 f>, record=false) at /home/marxin/Programming/gcc/gcc/cp/decl.c:1118

is called with record=false. So that cgraph_node is not created for it.

Or is a possible solution that get_function_version_dispatcher should look through the DECL_LOCAL_DECL_ALIAS?

Thanks,
Martin

> 
> Jason
> 

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

On 3/2/21 5:34 AM, Martin Liška wrote:
> On 3/1/21 8:58 PM, Jason Merrill wrote:
>> On 3/1/21 11:59 AM, Martin Liška wrote:
>>> On 3/1/21 5:36 PM, Jason Merrill wrote:
>>>> On 3/1/21 7:43 AM, Martin Liška wrote:
>>>>> On 2/22/21 11:53 PM, Jason Merrill wrote:
>>>>>> The problem seems to be with the handling of local decls.  If 
>>>>>> DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to 
>>>>>> find the namespace-scope decl.  But then if there is no preceding 
>>>>>> namespace-scope declaration, the new decl created by 
>>>>>> push_local_extern_decl_alias doesn't have a cgraph node, either.  
>>>>>> I guess maybe_function_versions 
>>>>>> also needs to look through DECL_LOCAL_DECL_ALIAS.
>>>>>
>>>>> Ah, I see. Are you sure about the name 'maybe_function_versions'? I 
>>>>> can't find it.
>>>>
>>>> Ah, it's maybe_version_functions, sorry.
>>>
>>> Thanks, I see the function now.
>>> So about your guess:
>>>
>>>> I guess maybe_function_versions also needs to look through 
>>>> DECL_LOCAL_DECL_ALIAS.
>>>
>>> Do you mean maybe_version_functions's argument 'record' should depend 
>>> on DECL_LOCAL_DECL_ALIAS of newdecl/oldddecl
>>> (if present)? Or that DECL_FUNCTION_VERSIONED should be set for 
>>> DECL_LOCAL_DECL_ALIASes of the newdecl/olddecl
>>> function declarations?
>>
>> The latter.
> 
> I see, but will not help us. Problem is that
> #2  0x00000000015d8899 in ix86_get_function_versions_dispatcher 
> (decl=0x7ffff7755000) at 
> /home/marxin/Programming/gcc/gcc/config/i386/i386-features.c:2862
> 
> is called for a declaration for which
> 
> Breakpoint 5, maybe_version_functions (newdecl=<function_decl 
> 0x7ffff7755000 f>, olddecl=<function_decl 0x7ffff7751f00 f>, 
> record=false) at /home/marxin/Programming/gcc/gcc/cp/decl.c:1118
> 
> is called with record=false. So that cgraph_node is not created for it.
> 
> Or is a possible solution that get_function_version_dispatcher should 
> look through the DECL_LOCAL_DECL_ALIAS?

Yes, I was thinking both that function and maybe_version_functions.

Jason

Re: [PATCH] C++: target attribute - local decl

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

On 3/2/21 6:57 PM, Jason Merrill wrote:
> On 3/2/21 5:34 AM, Martin Liška wrote:
>> On 3/1/21 8:58 PM, Jason Merrill wrote:
>>> On 3/1/21 11:59 AM, Martin Liška wrote:
>>>> On 3/1/21 5:36 PM, Jason Merrill wrote:
>>>>> On 3/1/21 7:43 AM, Martin Liška wrote:
>>>>>> On 2/22/21 11:53 PM, Jason Merrill wrote:
>>>>>>> The problem seems to be with the handling of local decls.  If DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to find the namespace-scope decl.  But then if there is no preceding namespace-scope declaration, the new decl created by push_local_extern_decl_alias doesn't have a cgraph node, either. I guess maybe_function_versions also needs to look through DECL_LOCAL_DECL_ALIAS.
>>>>>>
>>>>>> Ah, I see. Are you sure about the name 'maybe_function_versions'? I can't find it.
>>>>>
>>>>> Ah, it's maybe_version_functions, sorry.
>>>>
>>>> Thanks, I see the function now.
>>>> So about your guess:
>>>>
>>>>> I guess maybe_function_versions also needs to look through DECL_LOCAL_DECL_ALIAS.
>>>>
>>>> Do you mean maybe_version_functions's argument 'record' should depend on DECL_LOCAL_DECL_ALIAS of newdecl/oldddecl
>>>> (if present)? Or that DECL_FUNCTION_VERSIONED should be set for DECL_LOCAL_DECL_ALIASes of the newdecl/olddecl
>>>> function declarations?
>>>
>>> The latter.
>>
>> I see, but will not help us. Problem is that
>> #2  0x00000000015d8899 in ix86_get_function_versions_dispatcher (decl=0x7ffff7755000) at /home/marxin/Programming/gcc/gcc/config/i386/i386-features.c:2862
>>
>> is called for a declaration for which
>>
>> Breakpoint 5, maybe_version_functions (newdecl=<function_decl 0x7ffff7755000 f>, olddecl=<function_decl 0x7ffff7751f00 f>, record=false) at /home/marxin/Programming/gcc/gcc/cp/decl.c:1118
>>
>> is called with record=false. So that cgraph_node is not created for it.
>>
>> Or is a possible solution that get_function_version_dispatcher should look through the DECL_LOCAL_DECL_ALIAS?
> 
> Yes, I was thinking both that function and maybe_version_functions.
> 
> Jason
> 

All right, the following patch fixes that.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

[-- Attachment #2: 0001-c-support-target-attr-for-DECL_LOCAL_DECL_P-fns.patch --]
[-- Type: text/x-patch, Size: 3865 bytes --]

From ffff192a6ad5840c425a940ac724e27e0d25f7b0 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>
Date: Wed, 3 Mar 2021 09:38:55 +0100
Subject: [PATCH] c++: support target attr for DECL_LOCAL_DECL_P fns

gcc/cp/ChangeLog:

	PR c++/99108
	* call.c (get_function_version_dispatcher): Understand
	DECL_LOCAL_DECL_ALIAS.
	* decl.c (record_function_versions): New.
	(maybe_version_functions): Call record_function_versions
	for both declarations and DECL_LOCAL_DECL_ALIAS aliases.

gcc/testsuite/ChangeLog:

	PR c++/99108
	* g++.target/i386/pr99108.C: New test.
---
 gcc/cp/call.c                           |  4 +++
 gcc/cp/decl.c                           | 40 +++++++++++++++----------
 gcc/testsuite/g++.target/i386/pr99108.C | 18 +++++++++++
 3 files changed, 47 insertions(+), 15 deletions(-)
 create mode 100644 gcc/testsuite/g++.target/i386/pr99108.C

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 123f06b1f2b..117f1755191 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8467,6 +8467,10 @@ get_function_version_dispatcher (tree fn)
 {
   tree dispatcher_decl = NULL;
 
+  if (DECL_LOCAL_DECL_P (fn)
+      && DECL_LOCAL_DECL_ALIAS (fn) != NULL_TREE)
+    fn = DECL_LOCAL_DECL_ALIAS (fn);
+
   gcc_assert (TREE_CODE (fn) == FUNCTION_DECL
 	      && DECL_FUNCTION_VERSIONED (fn));
 
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 1742e286d9f..69bf3011a5a 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1108,6 +1108,23 @@ decls_match (tree newdecl, tree olddecl, bool record_versions /* = true */)
   return types_match;
 }
 
+/* Register function version for DECL that would point to VERSION.
+   If RECORD set to true, register function version in call graph.  */
+
+static void
+record_function_versions (tree decl, tree version, bool record)
+{
+  if (!DECL_FUNCTION_VERSIONED (version))
+    {
+      DECL_FUNCTION_VERSIONED (version) = 1;
+      if (DECL_ASSEMBLER_NAME_SET_P (version))
+	mangle_decl (version);
+    }
+
+  if (record && decl != version)
+    cgraph_node::record_function_versions (decl, version);
+}
+
 /* NEWDECL and OLDDECL have identical signatures.  If they are
    different versions adjust them and return true.
    If RECORD is set to true, record function versions.  */
@@ -1118,22 +1135,15 @@ maybe_version_functions (tree newdecl, tree olddecl, bool record)
   if (!targetm.target_option.function_versions (newdecl, olddecl))
     return false;
 
-  if (!DECL_FUNCTION_VERSIONED (olddecl))
-    {
-      DECL_FUNCTION_VERSIONED (olddecl) = 1;
-      if (DECL_ASSEMBLER_NAME_SET_P (olddecl))
-	mangle_decl (olddecl);
-    }
-
-  if (!DECL_FUNCTION_VERSIONED (newdecl))
-    {
-      DECL_FUNCTION_VERSIONED (newdecl) = 1;
-      if (DECL_ASSEMBLER_NAME_SET_P (newdecl))
-	mangle_decl (newdecl);
-    }
+  record_function_versions (olddecl, olddecl, record);
+  if (DECL_LOCAL_DECL_P (olddecl)
+      && DECL_LOCAL_DECL_ALIAS (olddecl) != NULL_TREE)
+    record_function_versions (olddecl, DECL_LOCAL_DECL_ALIAS (olddecl), true);
 
-  if (record)
-    cgraph_node::record_function_versions (olddecl, newdecl);
+  record_function_versions (olddecl, newdecl, record);
+  if (DECL_LOCAL_DECL_P (newdecl)
+      && DECL_LOCAL_DECL_ALIAS (newdecl) != NULL_TREE)
+    record_function_versions (olddecl, DECL_LOCAL_DECL_ALIAS (newdecl), true);
 
   return true;
 }
diff --git a/gcc/testsuite/g++.target/i386/pr99108.C b/gcc/testsuite/g++.target/i386/pr99108.C
new file mode 100644
index 00000000000..ad9409b8799
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/pr99108.C
@@ -0,0 +1,18 @@
+/* PR c++/99108 */
+/* { dg-do compile { target c++20 } } */
+/* { dg-require-ifunc "" }  */
+
+struct A {
+  void foo(auto);
+};
+void A::foo(auto)
+{
+  int f(void) __attribute__((target("default")));
+  int f(void) __attribute__((target("arch=atom")));
+  int b = f();
+}
+void bar(void)
+{
+  A c;
+  c.foo(7);
+}
-- 
2.30.1

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

On 3/4/21 3:19 AM, Martin Liška wrote:
> On 3/2/21 6:57 PM, Jason Merrill wrote:
>> On 3/2/21 5:34 AM, Martin Liška wrote:
>>> On 3/1/21 8:58 PM, Jason Merrill wrote:
>>>> On 3/1/21 11:59 AM, Martin Liška wrote:
>>>>> On 3/1/21 5:36 PM, Jason Merrill wrote:
>>>>>> On 3/1/21 7:43 AM, Martin Liška wrote:
>>>>>>> On 2/22/21 11:53 PM, Jason Merrill wrote:
>>>>>>>> The problem seems to be with the handling of local decls.  If 
>>>>>>>> DECL_LOCAL_DECL_P, you need to look at DECL_LOCAL_DECL_ALIAS to 
>>>>>>>> find the namespace-scope decl.  But then if there is no 
>>>>>>>> preceding namespace-scope declaration, the new decl created by 
>>>>>>>> push_local_extern_decl_alias doesn't have a cgraph node, either. 
>>>>>>>> I guess maybe_function_versions 
>>>>>>>> also needs to look through DECL_LOCAL_DECL_ALIAS.
>>>>>>>
>>>>>>> Ah, I see. Are you sure about the name 'maybe_function_versions'? 
>>>>>>> I can't find it.
>>>>>>
>>>>>> Ah, it's maybe_version_functions, sorry.
>>>>>
>>>>> Thanks, I see the function now.
>>>>> So about your guess:
>>>>>
>>>>>> I guess maybe_function_versions also needs to look through 
>>>>>> DECL_LOCAL_DECL_ALIAS.
>>>>>
>>>>> Do you mean maybe_version_functions's argument 'record' should 
>>>>> depend on DECL_LOCAL_DECL_ALIAS of newdecl/oldddecl
>>>>> (if present)? Or that DECL_FUNCTION_VERSIONED should be set for 
>>>>> DECL_LOCAL_DECL_ALIASes of the newdecl/olddecl
>>>>> function declarations?
>>>>
>>>> The latter.
>>>
>>> I see, but will not help us. Problem is that
>>> #2  0x00000000015d8899 in ix86_get_function_versions_dispatcher 
>>> (decl=0x7ffff7755000) at 
>>> /home/marxin/Programming/gcc/gcc/config/i386/i386-features.c:2862
>>>
>>> is called for a declaration for which
>>>
>>> Breakpoint 5, maybe_version_functions (newdecl=<function_decl 
>>> 0x7ffff7755000 f>, olddecl=<function_decl 0x7ffff7751f00 f>, 
>>> record=false) at /home/marxin/Programming/gcc/gcc/cp/decl.c:1118
>>>
>>> is called with record=false. So that cgraph_node is not created for it.
>>>
>>> Or is a possible solution that get_function_version_dispatcher should 
>>> look through the DECL_LOCAL_DECL_ALIAS?
>>
>> Yes, I was thinking both that function and maybe_version_functions.
>>
>> Jason
>>
> 
> All right, the following patch fixes that.
> 
> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

> +  record_function_versions (olddecl, olddecl, record);
> +  if (DECL_LOCAL_DECL_P (olddecl)
> +      && DECL_LOCAL_DECL_ALIAS (olddecl) != NULL_TREE)
> +    record_function_versions (olddecl, DECL_LOCAL_DECL_ALIAS (olddecl), true);
>  
> +  record_function_versions (olddecl, newdecl, record);
> +  if (DECL_LOCAL_DECL_P (newdecl)
> +      && DECL_LOCAL_DECL_ALIAS (newdecl) != NULL_TREE)
> +    record_function_versions (olddecl, DECL_LOCAL_DECL_ALIAS (newdecl), true);

Do you really need to register all these pairs?  I was expecting that 
you'd just look through DECL_LOCAL_DECL_ALIAS and then register the pair 
you end up with.  Local decls shouldn't need a cgraph node.

Jason

Re: [PATCH] C++: target attribute - local decl

[Martin Liška]

On 3/4/21 4:03 PM, Jason Merrill wrote:
> Do you really need to register all these pairs?  I was expecting that you'd just look through DECL_LOCAL_DECL_ALIAS and then register the pair you end up with.  Local decls shouldn't need a cgraph node.

Well, doing that:


commit 662c486edafa467ec41091e84857d594eaf280a2
Author: Martin Liska <mliska@suse.cz>
Date:   Wed Mar 3 09:38:55 2021 +0100

     c++: support target attr for DECL_LOCAL_DECL_P fns
     
     gcc/cp/ChangeLog:
     
             PR c++/99108
             * call.c (get_function_version_dispatcher): Understand
             DECL_LOCAL_DECL_ALIAS.
             * decl.c (record_function_versions): New.
             (maybe_version_functions): Call record_function_versions
             for both declarations and DECL_LOCAL_DECL_ALIAS aliases.
     
     gcc/testsuite/ChangeLog:
     
             PR c++/99108
             * g++.target/i386/pr99108.C: New test.

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 123f06b1f2b..117f1755191 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8467,6 +8467,10 @@ get_function_version_dispatcher (tree fn)
  {
    tree dispatcher_decl = NULL;
  
+  if (DECL_LOCAL_DECL_P (fn)
+      && DECL_LOCAL_DECL_ALIAS (fn) != NULL_TREE)
+    fn = DECL_LOCAL_DECL_ALIAS (fn);
+
    gcc_assert (TREE_CODE (fn) == FUNCTION_DECL
  	      && DECL_FUNCTION_VERSIONED (fn));
  
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 1742e286d9f..a092539bfa0 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1115,6 +1115,14 @@ decls_match (tree newdecl, tree olddecl, bool record_versions /* = true */)
  bool
  maybe_version_functions (tree newdecl, tree olddecl, bool record)
  {
+  if (DECL_LOCAL_DECL_P (olddecl)
+      && DECL_LOCAL_DECL_ALIAS (olddecl) != NULL_TREE)
+    olddecl = DECL_LOCAL_DECL_ALIAS (olddecl);
+
+  if (DECL_LOCAL_DECL_P (newdecl)
+      && DECL_LOCAL_DECL_ALIAS (newdecl) != NULL_TREE)
+    newdecl = DECL_LOCAL_DECL_ALIAS (newdecl);
+
    if (!targetm.target_option.function_versions (newdecl, olddecl))
      return false;

The compilation then fails:
/home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C: In member function ‘void A::foo(auto:2)’:
/home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C:12:13: error: call of overloaded ‘f()’ is ambiguous
    12 |   int b = f();
       |             ^
/home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C:10:7: note: candidate: ‘int f()’
    10 |   int f(void) __attribute__((target("default")));
       |       ^
/home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C:11:7: note: candidate: ‘int f()’
    11 |   int f(void) __attribute__((target("arch=atom")));
       |       ^

likely because DECL_FUNCTION_VERSIONED is missing for the local decls.

Martin

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

On 3/4/21 10:39 AM, Martin Liška wrote:
> On 3/4/21 4:03 PM, Jason Merrill wrote:
>> Do you really need to register all these pairs?  I was expecting that 
>> you'd just look through DECL_LOCAL_DECL_ALIAS and then register the 
>> pair you end up with.  Local decls shouldn't need a cgraph node.
> 
> Well, doing that:
> 
> 
> commit 662c486edafa467ec41091e84857d594eaf280a2
> Author: Martin Liska <mliska@suse.cz>
> Date:   Wed Mar 3 09:38:55 2021 +0100
> 
>      c++: support target attr for DECL_LOCAL_DECL_P fns
>      gcc/cp/ChangeLog:
>              PR c++/99108
>              * call.c (get_function_version_dispatcher): Understand
>              DECL_LOCAL_DECL_ALIAS.
>              * decl.c (record_function_versions): New.
>              (maybe_version_functions): Call record_function_versions
>              for both declarations and DECL_LOCAL_DECL_ALIAS aliases.
>      gcc/testsuite/ChangeLog:
>              PR c++/99108
>              * g++.target/i386/pr99108.C: New test.
> 
> diff --git a/gcc/cp/call.c b/gcc/cp/call.c
> index 123f06b1f2b..117f1755191 100644
> --- a/gcc/cp/call.c
> +++ b/gcc/cp/call.c
> @@ -8467,6 +8467,10 @@ get_function_version_dispatcher (tree fn)
>   {
>     tree dispatcher_decl = NULL;
> 
> +  if (DECL_LOCAL_DECL_P (fn)
> +      && DECL_LOCAL_DECL_ALIAS (fn) != NULL_TREE)
> +    fn = DECL_LOCAL_DECL_ALIAS (fn);
> +
>     gcc_assert (TREE_CODE (fn) == FUNCTION_DECL
>             && DECL_FUNCTION_VERSIONED (fn));
> 
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index 1742e286d9f..a092539bfa0 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -1115,6 +1115,14 @@ decls_match (tree newdecl, tree olddecl, bool 
> record_versions /* = true */)
>   bool
>   maybe_version_functions (tree newdecl, tree olddecl, bool record)
>   {
> +  if (DECL_LOCAL_DECL_P (olddecl)
> +      && DECL_LOCAL_DECL_ALIAS (olddecl) != NULL_TREE)
> +    olddecl = DECL_LOCAL_DECL_ALIAS (olddecl);
> +
> +  if (DECL_LOCAL_DECL_P (newdecl)
> +      && DECL_LOCAL_DECL_ALIAS (newdecl) != NULL_TREE)
> +    newdecl = DECL_LOCAL_DECL_ALIAS (newdecl);
> +
>     if (!targetm.target_option.function_versions (newdecl, olddecl))
>       return false;
> 
> The compilation then fails:
> /home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C: In 
> member function ‘void A::foo(auto:2)’:
> /home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C:12:13: 
> error: call of overloaded ‘f()’ is ambiguous
>     12 |   int b = f();
>        |             ^
> /home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C:10:7: 
> note: candidate: ‘int f()’
>     10 |   int f(void) __attribute__((target("default")));
>        |       ^
> /home/marxin/Programming/gcc/gcc/testsuite/g++.target/i386/pr99108.C:11:7: 
> note: candidate: ‘int f()’
>     11 |   int f(void) __attribute__((target("arch=atom")));
>        |       ^
> 
> likely because DECL_FUNCTION_VERSIONED is missing for the local decls.

Sure, I guess you do need to set that flag for the local decls, but that 
should be all.

Jason

Re: [PATCH] C++: target attribute - local decl

[Martin Liška]

On 3/4/21 4:45 PM, Jason Merrill wrote:
> 
> Sure, I guess you do need to set that flag for the local decls, but that should be all.
> 
> Jason

Doing that also fails :/
This time likely due to how we set RECORD argument of maybe_version_functions function:

gcc/cp/decl.c:    && maybe_version_functions (newdecl, olddecl,
gcc/cp/decl.c-                                (!DECL_FUNCTION_VERSIONED (newdecl)
gcc/cp/decl.c-                                 || !DECL_FUNCTION_VERSIONED (olddecl))))

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 123f06b1f2b..117f1755191 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8467,6 +8467,10 @@ get_function_version_dispatcher (tree fn)
  {
    tree dispatcher_decl = NULL;
  
+  if (DECL_LOCAL_DECL_P (fn)
+      && DECL_LOCAL_DECL_ALIAS (fn) != NULL_TREE)
+    fn = DECL_LOCAL_DECL_ALIAS (fn);
+
    gcc_assert (TREE_CODE (fn) == FUNCTION_DECL
  	      && DECL_FUNCTION_VERSIONED (fn));
  
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 1742e286d9f..4fa9c2a0dcb 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1108,6 +1108,17 @@ decls_match (tree newdecl, tree olddecl, bool record_versions /* = true */)
    return types_match;
  }
  
+static void
+maybe_mark_function_versioned (tree decl)
+{
+  if (!DECL_FUNCTION_VERSIONED (decl))
+    {
+      DECL_FUNCTION_VERSIONED (decl) = 1;
+      if (DECL_ASSEMBLER_NAME_SET_P (decl))
+	mangle_decl (decl);
+    }
+}
+
  /* NEWDECL and OLDDECL have identical signatures.  If they are
     different versions adjust them and return true.
     If RECORD is set to true, record function versions.  */
@@ -1118,18 +1129,20 @@ maybe_version_functions (tree newdecl, tree olddecl, bool record)
    if (!targetm.target_option.function_versions (newdecl, olddecl))
      return false;
  
-  if (!DECL_FUNCTION_VERSIONED (olddecl))
+  maybe_mark_function_versioned (olddecl);
+  if (DECL_LOCAL_DECL_P (olddecl)
+      && DECL_LOCAL_DECL_ALIAS (olddecl) != NULL_TREE)
      {
-      DECL_FUNCTION_VERSIONED (olddecl) = 1;
-      if (DECL_ASSEMBLER_NAME_SET_P (olddecl))
-	mangle_decl (olddecl);
+      olddecl = DECL_LOCAL_DECL_ALIAS (olddecl);
+      maybe_mark_function_versioned (olddecl);
      }
  
-  if (!DECL_FUNCTION_VERSIONED (newdecl))
+  maybe_mark_function_versioned (newdecl);
+  if (DECL_LOCAL_DECL_P (newdecl)
+      && DECL_LOCAL_DECL_ALIAS (newdecl) != NULL_TREE)
      {
-      DECL_FUNCTION_VERSIONED (newdecl) = 1;
-      if (DECL_ASSEMBLER_NAME_SET_P (newdecl))
-	mangle_decl (newdecl);
+      newdecl = DECL_LOCAL_DECL_ALIAS (newdecl);
+      maybe_mark_function_versioned (newdecl);
      }
  
    if (record)
diff --git a/gcc/testsuite/g++.target/i386/pr99108.C b/gcc/testsuite/g++.target/i386/pr99108.C
new file mode 100644
index 00000000000..ad9409b8799
--- /dev/null
+++ b/gcc/testsuite/g++.target/i386/pr99108.C
@@ -0,0 +1,18 @@
+/* PR c++/99108 */
+/* { dg-do compile { target c++20 } } */
+/* { dg-require-ifunc "" }  */
+
+struct A {
+  void foo(auto);
+};
+void A::foo(auto)
+{
+  int f(void) __attribute__((target("default")));
+  int f(void) __attribute__((target("arch=atom")));
+  int b = f();
+}
+void bar(void)
+{
+  A c;
+  c.foo(7);
+}

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

On 3/4/21 10:52 AM, Martin Liška wrote:
> On 3/4/21 4:45 PM, Jason Merrill wrote:
>>
>> Sure, I guess you do need to set that flag for the local decls, but 
>> that should be all.
>>
>> Jason
> 
> Doing that also fails :/
> This time likely due to how we set RECORD argument of 
> maybe_version_functions function:
> 
> gcc/cp/decl.c:    && maybe_version_functions (newdecl, olddecl,
> gcc/cp/decl.c-                                (!DECL_FUNCTION_VERSIONED 
> (newdecl)
> gcc/cp/decl.c-                                 || 
> !DECL_FUNCTION_VERSIONED (olddecl))))

That is odd.

The other problem is that DECL_LOCAL_DECL_ALIAS isn't always set before 
we get to maybe_version_functions; it's set further down in do_pushdecl. 
  We need it to be set or things break.

This seems to work for me, what do you think?

BTW, your patch was corrupted by the mailer, so I had to apply it by hand.

[-- Attachment #2: 0001-alias-version.patch --]
[-- Type: text/x-patch, Size: 3778 bytes --]

From 4227d0906d5e73d80cc0500e72e6277474948911 Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
Date: Thu, 4 Mar 2021 11:29:48 -0500
Subject: [PATCH] alias-version
To: gcc-patches@gcc.gnu.org

---
 gcc/cp/name-lookup.h |  1 +
 gcc/cp/call.c        |  3 +++
 gcc/cp/decl.c        | 29 +++++++++++++++++++++--------
 gcc/cp/name-lookup.c |  4 ++--
 4 files changed, 27 insertions(+), 10 deletions(-)

diff --git a/gcc/cp/name-lookup.h b/gcc/cp/name-lookup.h
index d95472b7545..fec95f67867 100644
--- a/gcc/cp/name-lookup.h
+++ b/gcc/cp/name-lookup.h
@@ -464,6 +464,7 @@ extern void cp_emit_debug_info_for_using (tree, tree);
 
 extern void finish_nonmember_using_decl (tree scope, tree name);
 extern void finish_using_directive (tree target, tree attribs);
+void push_local_extern_decl_alias (tree decl);
 extern tree pushdecl (tree, bool hiding = false);
 extern tree pushdecl_outermost_localscope (tree);
 extern tree pushdecl_top_level (tree);
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 123f06b1f2b..33b77870644 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8467,6 +8467,9 @@ get_function_version_dispatcher (tree fn)
 {
   tree dispatcher_decl = NULL;
 
+  if (DECL_LOCAL_DECL_P (fn))
+    fn = DECL_LOCAL_DECL_ALIAS (fn);
+
   gcc_assert (TREE_CODE (fn) == FUNCTION_DECL
 	      && DECL_FUNCTION_VERSIONED (fn));
 
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index 1742e286d9f..29461ad60b2 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -1108,6 +1108,17 @@ decls_match (tree newdecl, tree olddecl, bool record_versions /* = true */)
   return types_match;
 }
 
+static void
+maybe_mark_function_versioned (tree decl)
+{
+  if (!DECL_FUNCTION_VERSIONED (decl))
+    {
+      DECL_FUNCTION_VERSIONED (decl) = 1;
+      if (DECL_ASSEMBLER_NAME_SET_P (decl))
+	mangle_decl (decl);
+    }
+}
+
 /* NEWDECL and OLDDECL have identical signatures.  If they are
    different versions adjust them and return true.
    If RECORD is set to true, record function versions.  */
@@ -1118,18 +1129,20 @@ maybe_version_functions (tree newdecl, tree olddecl, bool record)
   if (!targetm.target_option.function_versions (newdecl, olddecl))
     return false;
 
-  if (!DECL_FUNCTION_VERSIONED (olddecl))
+  maybe_mark_function_versioned (olddecl);
+  if (DECL_LOCAL_DECL_P (olddecl))
     {
-      DECL_FUNCTION_VERSIONED (olddecl) = 1;
-      if (DECL_ASSEMBLER_NAME_SET_P (olddecl))
-	mangle_decl (olddecl);
+      olddecl = DECL_LOCAL_DECL_ALIAS (olddecl);
+      maybe_mark_function_versioned (olddecl);
     }
 
-  if (!DECL_FUNCTION_VERSIONED (newdecl))
+  maybe_mark_function_versioned (newdecl);
+  if (DECL_LOCAL_DECL_P (newdecl))
     {
-      DECL_FUNCTION_VERSIONED (newdecl) = 1;
-      if (DECL_ASSEMBLER_NAME_SET_P (newdecl))
-	mangle_decl (newdecl);
+      if (!DECL_LOCAL_DECL_ALIAS (newdecl))
+	push_local_extern_decl_alias (newdecl);
+      newdecl = DECL_LOCAL_DECL_ALIAS (newdecl);
+      maybe_mark_function_versioned (newdecl);
     }
 
   if (record)
diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index 66c35a1c16d..bd7fd1688d8 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -3384,7 +3384,7 @@ set_decl_context_in_fn (tree ctx, tree decl)
 /* DECL is a local extern decl.  Find or create the namespace-scope
    decl that it aliases.  Also, determines the linkage of DECL.  */
 
-static void
+void
 push_local_extern_decl_alias (tree decl)
 {
   if (dependent_type_p (TREE_TYPE (decl)))
@@ -3418,7 +3418,7 @@ push_local_extern_decl_alias (tree decl)
 
       if (binding && TREE_CODE (binding) != TREE_LIST)
 	for (ovl_iterator iter (binding); iter; ++iter)
-	  if (decls_match (*iter, decl))
+	  if (decls_match (decl, *iter, /*record_versions*/false))
 	    {
 	      alias = *iter;
 	      break;
-- 
2.27.0

Re: [PATCH] C++: target attribute - local decl

[Martin Liška]

On 3/4/21 9:54 PM, Jason Merrill wrote:
> On 3/4/21 10:52 AM, Martin Liška wrote:
>> On 3/4/21 4:45 PM, Jason Merrill wrote:
>>>
>>> Sure, I guess you do need to set that flag for the local decls, but that should be all.
>>>
>>> Jason
>>
>> Doing that also fails :/
>> This time likely due to how we set RECORD argument of maybe_version_functions function:
>>
>> gcc/cp/decl.c:    && maybe_version_functions (newdecl, olddecl,
>> gcc/cp/decl.c-                                (!DECL_FUNCTION_VERSIONED (newdecl)
>> gcc/cp/decl.c-                                 || !DECL_FUNCTION_VERSIONED (olddecl))))
> 
> That is odd.
> 
> The other problem is that DECL_LOCAL_DECL_ALIAS isn't always set before we get to maybe_version_functions; it's set further down in do_pushdecl.  We need it to be set or things break.

Oh, I see.

> 
> This seems to work for me, what do you think?

Seems good to me, please apply the patch.

> 
> BTW, your patch was corrupted by the mailer, so I had to apply it by hand.

I know, I made a quick dirty copy&paste ... sorry.

Thanks for help,
Martin

Re: [PATCH] C++: target attribute - local decl

[Jason Merrill]

On 3/8/21 4:33 AM, Martin Liška wrote:
> On 3/4/21 9:54 PM, Jason Merrill wrote:
>> On 3/4/21 10:52 AM, Martin Liška wrote:
>>> On 3/4/21 4:45 PM, Jason Merrill wrote:
>>>>
>>>> Sure, I guess you do need to set that flag for the local decls, but 
>>>> that should be all.
>>>>
>>>> Jason
>>>
>>> Doing that also fails :/
>>> This time likely due to how we set RECORD argument of 
>>> maybe_version_functions function:
>>>
>>> gcc/cp/decl.c:    && maybe_version_functions (newdecl, olddecl,
>>> gcc/cp/decl.c-                                
>>> (!DECL_FUNCTION_VERSIONED (newdecl)
>>> gcc/cp/decl.c-                                 || 
>>> !DECL_FUNCTION_VERSIONED (olddecl))))
>>
>> That is odd.
>>
>> The other problem is that DECL_LOCAL_DECL_ALIAS isn't always set 
>> before we get to maybe_version_functions; it's set further down in 
>> do_pushdecl.  We need it to be set or things break.
> 
> Oh, I see.
> 
>>
>> This seems to work for me, what do you think?
> 
> Seems good to me, please apply the patch.

Done.

Jason