[Bug ipa/106816] New: noreturn/pure attributes are not set correctly on multiversioned functions

[Bug ipa/106816] New: noreturn/pure attributes are not set correctly on multiversioned functions

[gcc.gnu at vvalter dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

            Bug ID: 106816
           Summary: noreturn/pure attributes are not set correctly on
                    multiversioned functions
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: ipa
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gcc.gnu at vvalter dot com
                CC: marxin at gcc dot gnu.org
  Target Milestone: ---

During the investigation of PR106627, Richard noted (
https://gcc.gnu.org/pipermail/gcc-patches/2022-September/600777.html ) that
attributes like noreturn and pure on multiversioned functions are lost when the
function declaration is replaced by the dispatcher declaration in the same way
that the TREE_NOTHROW attribute got lost, which was fixed in PR106627.

Example:

__attribute__((noreturn,target("default")))
void f() {
    for (;;) {}
}

__attribute__((noreturn,target("sse4.2,bmi")))
void f() {
    for (;;) {}
}

int main()
{
    f();
    return 1;
}


Gcc should create no code after the call to f(), but the assembly output with
-O3 looks like the following:

        call    _Z5_Z1fvv@PLT
        movl    $1, %eax
        addq    $8, %rsp
        .cfi_def_cfa_offset 8
        ret

For a non-multiversioned function, no assembly instructions are generated after
the call instruction. Similar problems happen if the function is marked pure.
I reproduced this on 11.2.0 and 12.2, but this most likely affects all gcc
versions.

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[hjl.tools at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-09-02
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #1 from H.J. Lu <hjl.tools at gmail dot com> ---
Like this?

diff --git a/gcc/config/i386/i386-features.cc
b/gcc/config/i386/i386-features.cc
index fd212262f50..4904e4d71b3 100644
--- a/gcc/config/i386/i386-features.cc
+++ b/gcc/config/i386/i386-features.cc
@@ -3269,6 +3269,9 @@ ix86_get_function_versions_dispatcher (void *decl)
       /* Right now, the dispatching is done via ifunc.  */
       dispatch_decl = make_dispatcher_decl (default_node->decl);
       TREE_NOTHROW (dispatch_decl) = TREE_NOTHROW (fn);
+      TREE_THIS_VOLATILE (dispatch_decl) = TREE_THIS_VOLATILE (fn);
+      TREE_READONLY (dispatch_decl) = TREE_READONLY (fn);
+      DECL_PURE_P (dispatch_decl) = DECL_PURE_P (fn);

       dispatcher_node = cgraph_node::get_create (dispatch_decl);
       gcc_assert (dispatcher_node != NULL);

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[gcc.gnu at vvalter dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #2 from Simon Rainer <gcc.gnu at vvalter dot com> ---
(In reply to H.J. Lu from comment #1)
> Like this?
> 
> diff --git a/gcc/config/i386/i386-features.cc
> b/gcc/config/i386/i386-features.cc
> index fd212262f50..4904e4d71b3 100644
> --- a/gcc/config/i386/i386-features.cc
> +++ b/gcc/config/i386/i386-features.cc
> @@ -3269,6 +3269,9 @@ ix86_get_function_versions_dispatcher (void *decl)
>        /* Right now, the dispatching is done via ifunc.  */
>        dispatch_decl = make_dispatcher_decl (default_node->decl);
>        TREE_NOTHROW (dispatch_decl) = TREE_NOTHROW (fn);
> +      TREE_THIS_VOLATILE (dispatch_decl) = TREE_THIS_VOLATILE (fn);
> +      TREE_READONLY (dispatch_decl) = TREE_READONLY (fn);
> +      DECL_PURE_P (dispatch_decl) = DECL_PURE_P (fn);
>  
>        dispatcher_node = cgraph_node::get_create (dispatch_decl);
>        gcc_assert (dispatcher_node != NULL);

I tried something like that, but I didn't see changes in the generated
assembly. I don't know if that is because something else is preventing
optimization or some other member needs to be set correctly.

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[hjl.tools at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #3 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to Simon Rainer from comment #2)
> (In reply to H.J. Lu from comment #1)
> > Like this?
> > 
> > diff --git a/gcc/config/i386/i386-features.cc
> > b/gcc/config/i386/i386-features.cc
> > index fd212262f50..4904e4d71b3 100644
> > --- a/gcc/config/i386/i386-features.cc
> > +++ b/gcc/config/i386/i386-features.cc
> > @@ -3269,6 +3269,9 @@ ix86_get_function_versions_dispatcher (void *decl)
> >        /* Right now, the dispatching is done via ifunc.  */
> >        dispatch_decl = make_dispatcher_decl (default_node->decl);
> >        TREE_NOTHROW (dispatch_decl) = TREE_NOTHROW (fn);
> > +      TREE_THIS_VOLATILE (dispatch_decl) = TREE_THIS_VOLATILE (fn);
> > +      TREE_READONLY (dispatch_decl) = TREE_READONLY (fn);
> > +      DECL_PURE_P (dispatch_decl) = DECL_PURE_P (fn);
> >  
> >        dispatcher_node = cgraph_node::get_create (dispatch_decl);
> >        gcc_assert (dispatcher_node != NULL);
> 
> I tried something like that, but I didn't see changes in the generated
> assembly. I don't know if that is because something else is preventing
> optimization or some other member needs to be set correctly.

I got

main:
.LFB2:
        .cfi_startproc
        subq    $8, %rsp
        .cfi_def_cfa_offset 16
        call    _Z5_Z1fvv
        .cfi_endproc

It looks correct.

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[gcc.gnu at vvalter dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #4 from Simon Rainer <gcc.gnu at vvalter dot com> ---
That's weird, I still get the following with your patch applied:

main:
.LFB2:
        .cfi_startproc
        subq    $8, %rsp
        .cfi_def_cfa_offset 16
        call    _Z5_Z1fvv@PLT
        movl    $1, %eax
        addq    $8, %rsp
        .cfi_def_cfa_offset 8
        ret
        .cfi_endproc

I double checked that and reran a full bootstrap, but maybe I'm doing something
wrong. I would also be surprised if information about volatile, readonly, and
pure are enough to detect that the function is noreturn, wouldn't that need to
be a separate information?

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
The function should probably inherit all of the IPA pure/const/modref analysis
result, that is all "IPA" state should be copied.  I think we want some helper
here - IPA clone creation must have something, no?

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #6 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> The function should probably inherit all of the IPA pure/const/modref
> analysis result, that is all "IPA" state should be copied.  I think we want
> some helper
> here - IPA clone creation must have something, no?

Well, in IPA clones utilize so-called function_summary that is a typical place
where we store analysis results. And we don't typically create a new tree
declarations when we clone cgraph_nodes (clones share the same FE declaration).

However, I noticed we want to do likely something similar to what cp/decl.c
does:

static void
merge_attribute_bits (tree newdecl, tree olddecl)
{
  TREE_THIS_VOLATILE (newdecl) |= TREE_THIS_VOLATILE (olddecl);
  TREE_THIS_VOLATILE (olddecl) |= TREE_THIS_VOLATILE (newdecl);
  TREE_NOTHROW (newdecl) |= TREE_NOTHROW (olddecl);
  TREE_NOTHROW (olddecl) |= TREE_NOTHROW (newdecl);
  TREE_READONLY (newdecl) |= TREE_READONLY (olddecl);
  TREE_READONLY (olddecl) |= TREE_READONLY (newdecl);
  DECL_IS_MALLOC (newdecl) |= DECL_IS_MALLOC (olddecl);
  DECL_IS_MALLOC (olddecl) |= DECL_IS_MALLOC (newdecl);
  DECL_PURE_P (newdecl) |= DECL_PURE_P (olddecl);
  DECL_PURE_P (olddecl) |= DECL_PURE_P (newdecl);
  DECL_UNINLINABLE (newdecl) |= DECL_UNINLINABLE (olddecl);
  DECL_UNINLINABLE (olddecl) |= DECL_UNINLINABLE (newdecl);
}

...

              /* Merge the noreturn bit.  */
              TREE_THIS_VOLATILE (olddecl) = TREE_THIS_VOLATILE (newdecl);
              TREE_READONLY (olddecl) = TREE_READONLY (newdecl);
              TREE_NOTHROW (olddecl) = TREE_NOTHROW (newdecl);
              DECL_IS_MALLOC (olddecl) = DECL_IS_MALLOC (newdecl);
              DECL_PURE_P (olddecl) = DECL_PURE_P (newdecl);

I can see IPA passes doing similar declaration clonning for VAR_DECLs
(gcc/ipa-param-manipulation.cc, gcc/tree-inline.c).

@Martin: Do we have a declaration cloning code for functions somewhere?

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |marxin at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #7 from Martin Liška <marxin at gcc dot gnu.org> ---
Let me assign it to myself.

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #8 from Martin Liška <marxin at gcc dot gnu.org> ---
> @Martin: Do we have a declaration cloning code for functions somewhere?

@jamborm: PING

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[jamborm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #9 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #6)
> 
> @Martin: Do we have a declaration cloning code for functions somewhere?

See e.g. cgraph_node::create_virtual_clone in cgraphclones.cc.  Unless
you want to mess with the parameters,

  new_decl = copy_node (old_decl);

should be enough (and it should copy over the DECL_PURE and
TREE_READLONY bit soo, I believe).

I am not sure at what point the duplication happens, in order to
duplicate also all the various IPA summaries, cgraph machinery has to
be involved and call all the summary hooks.  So
cgraph_node::create_clone should probably be used (or a part of it, or
perhaps even one of its users?).

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

--- Comment #10 from Martin Liška <marxin at gcc dot gnu.org> ---
(In reply to Martin Jambor from comment #9)
> (In reply to Martin Liška from comment #6)
> > 
> > @Martin: Do we have a declaration cloning code for functions somewhere?
> 
> See e.g. cgraph_node::create_virtual_clone in cgraphclones.cc.  Unless
> you want to mess with the parameters,
> 
>   new_decl = copy_node (old_decl);
> 
> should be enough (and it should copy over the DECL_PURE and
> TREE_READLONY bit soo, I believe).

Thanks! So apparently, we call make_dispatcher_decl which makes only a partial
copy of a function_decl:

original decl:

 <function_decl 0x7ffff77ce500 f
    type <function_type 0x7ffff7656d20
        type <void_type 0x7ffff7644f18 void VOID
            align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7ffff7644f18
            pointer_to_this <pointer_type 0x7ffff764c000>>
        QI
        size <integer_cst 0x7ffff76460c0 constant 8>
        unit-size <integer_cst 0x7ffff76460d8 constant 1>
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7ffff7656d20
        arg-types <tree_list 0x7ffff7639d98 value <void_type 0x7ffff7644f18
void>>
        pointer_to_this <pointer_type 0x7ffff77ee498>>
    volatile nothrow public static decl_5 QI
/home/marxin/Programming/testcases/pr106816.c:2:6 align:8 warn_if_not_align:0
context <translation_unit_decl 0x7ffff762d168
/home/marxin/Programming/testcases/pr106816.c>
    attributes <tree_list 0x7ffff77de0c8
        purpose <identifier_node 0x7ffff77caa40 target
            normal local bindings <(nil)>>
        value <tree_list 0x7ffff77de078
            value <string_cst 0x7ffff77c29e0 type <array_type 0x7ffff77ccb28>
                readonly constant static "default\000">>
        chain <tree_list 0x7ffff77de0a0
            purpose <identifier_node 0x7ffff7650500 noreturn
                normal local bindings <(nil)>>>> initial <block 0x7ffff77d1420>
    result <result_decl 0x7ffff762df78 D.2761 type <void_type 0x7ffff7644f18
void>
        ignored VOID /home/marxin/Programming/testcases/pr106816.c:2:1
        align:8 warn_if_not_align:0 context <function_decl 0x7ffff77ce500 f>>
    full-name "void f()"
    struct-function 0x7ffff77df000 chain <function_decl 0x7ffff77cbc00
__cxa_call_unexpected>>

dispatcher:

(gdb) p debug_tree(func_decl)
 <function_decl 0x7ffff77efd00 _Z1fv
    type <function_type 0x7ffff7656d20
        type <void_type 0x7ffff7644f18 void VOID
            align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7ffff7644f18
            pointer_to_this <pointer_type 0x7ffff764c000>>
        QI
        size <integer_cst 0x7ffff76460c0 constant 8>
        unit-size <integer_cst 0x7ffff76460d8 constant 1>
        align:8 warn_if_not_align:0 symtab:0 alias-set -1 canonical-type
0x7ffff7656d20
        arg-types <tree_list 0x7ffff7639d98 value <void_type 0x7ffff7644f18
void>>
        pointer_to_this <pointer_type 0x7ffff77ee498>>
    nothrow public external QI
/home/marxin/Programming/testcases/pr106816.c:15:1 align:8 warn_if_not_align:0>

So we modify the name, drop 'target' attribute, and so on. Thus we can't use
copy_node.


> 
> I am not sure at what point the duplication happens, in order to
> duplicate also all the various IPA summaries, cgraph machinery has to
> be involved and call all the summary hooks.  So
> cgraph_node::create_clone should probably be used (or a part of it, or
> perhaps even one of its users?).

It can happen during the multiple_target.cc IPA pass, so lemme combine the
H.J.'s approach
and cgraph_node::create_clone.

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.0
           Keywords|                            |patch

--- Comment #11 from Martin Liška <marxin at gcc dot gnu.org> ---
Patch candidate:
https://gcc.gnu.org/pipermail/gcc-patches/2023-April/615049.html

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.0                        |13.2

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 13.1 is being released, retargeting bugs to GCC 13.2.

[Bug ipa/106816] noreturn/pure attributes are not set correctly on multiversioned functions

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106816

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.2                        |13.3

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 13.2 is being released, retargeting bugs to GCC 13.3.