[Bug ipa/61190] [4.8/4.9/4.10 Regression] g++.old-deja/g++.mike/p4736b.C FAILs at -O2/-Os/-O3

["bernd.edlinger at hotmail dot de" <gcc-bugzilla@gcc.gnu.org>]

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

Bernd Edlinger <bernd.edlinger at hotmail dot de> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org

--- Comment #4 from Bernd Edlinger <bernd.edlinger at hotmail dot de> ---
(In reply to Jakub Jelinek from comment #3)
> That looks like a too ugly hack.  Much better is just to teach the
> ipa-pure-const pass about thunks IMHO, what they can and can't access.

yes, sigh, I know...

But OTOH this patch not only fixes the test case, but also
allows several optimizations on the thunk,
that were not possible before:

- inlining the function into the thunk
- inlining the thunk into the caller
- versioning on the thunk
- general optimizations on the thunk


this means, in this example:
 the thunk's access to the vtable is optimized away
 and then the constructor is also optimized away.
 => everything is just fine!

And, if inlining of the function into the thunk is
not possible for any reason, then I still see a
tail-call optimization.


Well I have still a few days off, to work on this.

What I tried so far is this:

Following Honzas advice, I tried this:

Index: ipa-pure-const.c
===================================================================
--- ipa-pure-const.c    (revision 212426)
+++ ipa-pure-const.c    (working copy)
@@ -735,6 +735,8 @@ analyze_function (struct cgraph_node *fn, bool ipa
   l->looping_previously_known = true;
   l->looping = false;
   l->can_throw = false;
+  if (fn->thunk.thunk_p && fn->thunk.virtual_offset_p)
+    l->pure_const_state = IPA_NEITHER;
   state_from_flags (&l->state_previously_known, &l->looping_previously_known,
             flags_from_decl_or_type (fn->decl),
             cgraph_node_cannot_return (fn));

BUT it does not fix the test case.  I frankly admit I do not fully
understand why...   Any insight here, might be helpful.



When I look at the code in ipa-pure-const.c,
I noticed, that cgraph_function_body_availability is called in many
places, and it may be able to influence lots of things.

So I tried this:

Index: cgraph.c
===================================================================
--- cgraph.c    (revision 212426)
+++ cgraph.c    (working copy)
@@ -2133,6 +2133,8 @@ cgraph_function_body_availability (struct cgraph_n
     avail = AVAIL_LOCAL;
   else if (node->alias && node->weakref)
     cgraph_function_or_thunk_node (node, &avail);
+  else if (node->thunk.thunk_p && node->thunk.virtual_offset_p)
+    avail = AVAIL_OVERWRITABLE;
   else if (lookup_attribute ("ifunc", DECL_ATTRIBUTES (node->decl)))
     avail = AVAIL_OVERWRITABLE;
   else if (!node->externally_visible)


And yes, this fixes the test case!

Returning AVAIL_OVERWRITABLE in this case means in my own words:
"this thunk can do anything, and you can not assume anything,
it may be even replaced by someting completely different, by the back-end."

However I don't know if this is a better soulution at all...

The resulting code, at -O2 is worse than with my first hacky patch,
because the constructor is now fully expanded, and can no longer be
optimized away, but it seems to work.

IMHO this issue clearly needs to be fixed, one way or the other...
What would be your ideas, how to fix it?


Thanks
Bernd.