[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[hubicka at gcc dot gnu.org]

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

Jan Hubicka <hubicka at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mjambor at suse dot cz

--- Comment #3 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
The testcase builds for me now, but this is Martin's code (apparently checking
that we did not forget to apply param adjustments)
Martin, was this fixed?

Honza

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[asolokha at gmx dot com]

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

--- Comment #4 from Arseny Solokha <asolokha at gmx dot com> ---
(In reply to Jan Hubicka from comment #3)
> The testcase builds for me now

It still ICEs for me.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[marxin at gcc dot gnu.org]

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

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
Yes, I can confirm it still ICEs.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |10.0, 9.3.0
      Known to work|                            |9.2.0

--- Comment #6 from Martin Liška <marxin at gcc dot gnu.org> ---
Apparently, GCC 9 branch is also affected since 9.3.0 release.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[marxin at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to work|9.2.0                       |9.3.0
      Known to fail|9.3.0                       |

--- Comment #7 from Martin Liška <marxin at gcc dot gnu.org> ---
No, I used bogus prebuilt binary.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

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

--- Comment #8 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Let me have a look

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

--- Comment #9 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #3)
> The testcase builds for me now, but this is Martin's code

that's questionable :-) Git blame points correctly to me but before
new IPA-SRA the assert used to be:

  gcc_assert (!node || !node->clone.combined_args_to_skip);

and was added by Honza in 2012 (in 66a20fc2a7de).

> (apparently
> checking that we did not forget to apply param adjustments)

AFAIU no, quite the opposite, it checks that we are not going to apply
param adjustment twice to a call, which is in a way what we are about
to do.

We find ourselves looking at a call statement with parameters already
adjusted and the decl in the statement being the IPA-CP created one.
In the cgraph edge, however, the callee's decl is one created during
save_inline_function_body.  Because redirect_call_stmt_to_callee
decides whether it has to do anything by comparing decls, it thinks it
has to redirect and remove params and... BOOM.

When I wrote that the call had already been adjusted that actually was
not entirely true.  The call was already created that way in
expand_thunk, because it is in an expanded artificial thunk of the
IPA-CP clone.

The assumption was that because the decl would be the correct one from
the start, no additional redirection would be taking place.  That
perhaps wasn't the best idea as save_inline_function_body can clearly
violate that (and in future some IPA pass might want to redirect the
edge too).

Having said that, I am not sure where to best fix this so late in the
GCC 10 development cycle.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[rguenth at gcc dot gnu.org]

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

--- Comment #10 from Richard Biener <rguenth at gcc dot gnu.org> ---
The offending commit eventually changed when/how many times
disable_insertion_hook () is called.  So that maybe points to the real cause.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[rguenth at gcc dot gnu.org]

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

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #10)
> The offending commit eventually changed when/how many times
> disable_insertion_hook () is called.  So that maybe points to the real cause.

Eventually that just triggered a different transform though.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[hubicka at ucw dot cz]

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

--- Comment #12 from Jan Hubicka <hubicka at ucw dot cz> ---
> Having said that, I am not sure where to best fix this so late in the
> GCC 10 development cycle.

So the problem is that thunk is expanded on the adjusted decl but we
still keep the adjustments and later fail to apply them?

I guess we have two options:
 1) force thunk expansion to happen on original decls (before cloning)
    so the body ends up being same as for ordinary function
 2) remove the adjustments after expansion - this should IMO work
    under the assumption that optimization passes don't insert
    non-trivial code into the thunk before they expand the thunk (i.e.
    if you want to adjust it in ipa-sra you will want to first produce
    the thunk and then do adjustement)
It seems to me that 2 should be not that hard to implement
Does that make sense?

Honza

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

--- Comment #13 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Jan Hubicka from comment #12)
> > Having said that, I am not sure where to best fix this so late in the
> > GCC 10 development cycle.
> 
> So the problem is that thunk is expanded on the adjusted decl but we
> still keep the adjustments and later fail to apply them?
> 
> I guess we have two options:
>  1) force thunk expansion to happen on original decls (before cloning)
>     so the body ends up being same as for ordinary function

I was thinking about this too.  I will try to look into expand_thunk
whether I can leave the call statement mostly alone (apart from the
thunk transform itself, of course).

>  2) remove the adjustments after expansion - this should IMO work
>     under the assumption that optimization passes don't insert
>     non-trivial code into the thunk before they expand the thunk (i.e.
>     if you want to adjust it in ipa-sra you will want to first produce
>     the thunk and then do adjustement)
> It seems to me that 2 should be not that hard to implement
> Does that make sense?

Unfortunately I don't think so.  The adjustment is attached to the
callee (just like in the past the skip_args bitmap was - and we're
only skipping arguments in the testcase), so you cannot just remove it
in one caller.  Or am I missing something?

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

--- Comment #14 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Actually, we should be able to simply skip applying adjustments, if
e->caller->former_thunk_p().  I'm playing with a patch.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

--- Comment #15 from Martin Jambor <jamborm at gcc dot gnu.org> ---
It turns out that no, recursive inlining will happily put an adjusted and not
yet adjusted call into the same function which was formerly a thunk.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

--- Comment #16 from Martin Jambor <jamborm at gcc dot gnu.org> ---
The following workaround works for the testcase but would need to be
generalized for a chain of former_decl_of's to be universal, I'm afraid:

diff --git a/gcc/cgraph.c b/gcc/cgraph.c
index 6b780f80eb3..241b996151a 100644
--- a/gcc/cgraph.c
+++ b/gcc/cgraph.c
@@ -1467,7 +1467,8 @@ cgraph_edge::redirect_call_stmt_to_callee (cgraph_edge
*e)


   if (e->indirect_unknown_callee
-      || decl == e->callee->decl)
+      || decl == e->callee->decl
+      || decl == e->callee->former_clone_of)
     return e->call_stmt;

   if (flag_checking && decl)
diff --git a/gcc/ipa-inline-transform.c b/gcc/ipa-inline-transform.c
index eed992d314d..a6675768552 100644
--- a/gcc/ipa-inline-transform.c
+++ b/gcc/ipa-inline-transform.c
@@ -588,6 +588,7 @@ save_inline_function_body (struct cgraph_node *node)
       first_clone->next_sibling_clone = NULL;
       gcc_assert (!first_clone->prev_sibling_clone);
     }
+  first_clone->former_clone_of = node->decl;
   first_clone->clone_of = NULL;

   /* Now node in question has no clones.  */

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

--- Comment #17 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Created attachment 48208
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48208&action=edit
WIP patch

This is the current version of my patch to fix this.  I think that at
least for the purposes of JIT I need to find a place to deallocate the
new summary - but that can only happen after all inlining is done.
Then I'll add that, re-base and submit it to the mailing list.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

--- Comment #18 from Martin Jambor <jamborm at gcc dot gnu.org> ---
I posted a patch to fix this for review to the mailing list:

https://gcc.gnu.org/pipermail/gcc-patches/2020-April/543659.html

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[cvs-commit at gcc dot gnu.org]

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

--- Comment #19 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Martin Jambor <jamborm@gcc.gnu.org>:

https://gcc.gnu.org/g:7123347c8b44571811c4b58506b06fb09969bccb

commit r10-7760-g7123347c8b44571811c4b58506b06fb09969bccb
Author: Martin Jambor <mjambor@suse.cz>
Date:   Thu Apr 16 19:21:02 2020 +0200

    ipa: Make call redirection detect already adjusted calls (PR 93621)

    PR 93621 testcase makes redirect_call_stmt_to_callee wrongly assume
    that a call statement needs redirecting but then rightly fails an
    assert ensuring the call statement parameters have not already been
    adjusted because they were already created adjusted as part of thunk
    expansion.

    The test fails because the decl in the call call statement is
    different than the decl of the callee, because the latter was created
    in save_inline_function_body.  This patch adds a way to link these two
    and detect the situation in redirect_call_stmt_to_callee.

    2020-04-16  Martin Jambor  <mjambor@suse.cz>

            PR ipa/93621
            * ipa-inline.h (ipa_saved_clone_sources): Declare.
            * ipa-inline-transform.c (ipa_saved_clone_sources): New variable.
            (save_inline_function_body): Link the new body holder with the
            previous one.
            * cgraph.c: Include ipa-inline.h.
            (cgraph_edge::redirect_call_stmt_to_callee): Try to find the decl
from
            the statement in ipa_saved_clone_sources.
            * cgraphunit.c: Include ipa-inline.h.
            (expand_all_functions): Free ipa_saved_clone_sources.

            testsuite/
            * g++.dg/ipa/pr93621.C: New test.

[Bug ipa/93621] [10 Regression] ICE in redirect_call_stmt_to_callee, at cgraph.c:1443 since r10-5567

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #20 from Martin Jambor <jamborm at gcc dot gnu.org> ---
Fixed for GCC 10, see the review email thread for caveats/future plans about
this.