Hi, testcase in this PR shows interesting bug in detect_type_change. The function basically looks up in the alises walk and tries to find explicit stored to vtable. If none are found the type is assumed to be fully built. If some are found and they are all agree, the type is assumed to be in the construction stage or fully built based on the vtable.
The code used to give up on first aliasing occurence of gimple_clobber. It seemed obvious change to not consider clobbers to be a statements changing dynamic type of memory location. http://gcc.gnu.org/ml/gcc-patches/2013-12/msg01331.html This change however triggers wrong code in the testcase attached. Here we have a loop with offline construction but inline destructor. As the code walks the aliasing statements it skips the construction and assumes that the destructoin is what always happens. The code makes interesting assumptions about how construction/destruction code is organized and basically have a thinko confusing the fact htat we found known vtable initialization at *one* path with the fact that the vtable initialization was found at all paths. The assumptions are described by Martin as follows: /* Return true if STMT can modify a virtual method table pointer. This function makes special assumptions about both constructors and destructors which are all the functions that are allowed to alter the VMT pointers. It assumes that destructors begin with assignment into all VMT pointers and that constructors essentially look in the following way: 1) The very first thing they do is that they call constructors of ancestor sub-objects that have them. 2) Then VMT pointers of this and all its ancestors is set to new values corresponding to the type corresponding to the constructor. 3) Only afterwards, other stuff such as constructor of member sub-objects and the code written by the user is run. Only this may include calling virtual functions, directly or indirectly. There is no way to call a constructor of an ancestor sub-object in any other way. This means that we do not have to care whether constructors get the correct type information because they will always change it (in fact, if we define the type to be given by the VMT pointer, it is undefined). The most important fact to derive from the above is that if, for some statement in the section 3, we try to detect whether the dynamic type has changed, we can safely ignore all calls as we examine the function body backwards until we reach statements in section 2 because these calls cannot be ancestor constructors or destructors (if the input is not bogus) and so do not change the dynamic type (this holds true only for automatically allocated objects but at the moment we devirtualize only these). We then must detect that statements in section 2 change the dynamic type and can try to derive the new type. That is enough and we can stop, we will never see the calls into constructors of sub-objects in this code. Therefore we can safely ignore all call statements that we traverse. */ I think his analysis is correct, but we need an alias walker to let us know if we found something along all paths or some paths. The code can also be strenghtened to actually understand calls that define type. I attached patch to the PR that works harder and discovers constructions/destructions explicitly and makes us to devirtualize quite a bit more. For 4.9 we however need a simple fix. I tried to simply give up on calls but that basically disables all the ipa-cp devirtualization and breaks half of devirt testsuite. On Firefox it makes us to devirtualize 90% less out of calls that we devirtualize by ipa-cp (that is about 600). I tired to make it weaker as Martin originally suggested, by considering only calls that may change the value. Those are only ctors/dtors and aggregate returns. This makes us still optimize 30% less and break good part of testsuite. Obviously driving IPA propagation by analysis that turns and runs away in a panic on first occurence of a function call is not going to work well. So instead I decided to simply revert the change to ignore clobbers. This restores the shape of function it was in for several releases, so it should be safe. I believe it can be shown safe on two premises. We always analyze only static/automatic vars (not heap storage). We thus know that all uses are dominated by construction and if you miss it, there is always clobber just before the destruction. I have couple plans for 4.10. First I want to extend the alias walker to let me know if top of function was reached, so this code can be also use for heap allocated storage. I however also think we want to make the changes explicit all the way from FE to IPA passes instead of doing this rather fragile pattern matching. I will send RFC proposal on this. Bootstrapped/regtested and comitted. Honza PR ipa/60306 Revert: 2013-12-14 Jan Hubicka <j...@suse.cz> PR middle-end/58477 * ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers. * testsuite/g++.dg/ipa/devirt-29.C: New testcase Index: ipa-prop.c =================================================================== --- ipa-prop.c (revision 208247) +++ ipa-prop.c (working copy) @@ -573,8 +573,7 @@ stmt_may_be_vtbl_ptr_store (gimple stmt) { if (is_gimple_call (stmt)) return false; - else if (gimple_clobber_p (stmt)) - return false; + /* TODO: Skip clobbers, doing so triggers problem in PR60306. */ else if (is_gimple_assign (stmt)) { tree lhs = gimple_assign_lhs (stmt); Index: testsuite/g++.dg/ipa/devirt-29.C =================================================================== --- testsuite/g++.dg/ipa/devirt-29.C (revision 0) +++ testsuite/g++.dg/ipa/devirt-29.C (revision 0) @@ -0,0 +1,75 @@ +/* { dg-do run } */ +/* There is a devirtualizable call. In PR60306 we deduced wrong target to cxa_pure_virtual. + For gcc 4.10 we temporarily disable the devirtualization. */ +/* { dg-options "-O3 -std=c++11" } */ + +#include <vector> + +using std::vector; + +class Object +{ +public: + + virtual Object* clone() const =0; + + virtual int type() const {return 0;} + + Object& operator=(const Object&) {return *this;} + + Object() {} + Object(const Object&) {} + virtual ~Object() {} +}; + +Object* f(const Object&o) +{ + return o.clone(); +} + +template<typename T> +class Box: public Object, public T +{ +public: + Box<T>* clone() const {return new Box<T>(*this);} + + Box<T>& operator=(const Box<T>& t) + { + T::operator=(t); + return *this; + } + + Box<T>& operator=(const T& t) + { + T::operator=(t); + return *this; + } + + Box() = default; + Box(const Box<T>&) = default; + explicit Box(const T& t):T(t) {} +}; + +template <typename T> +using Vector = Box<vector<T>>; + +typedef Vector<int> OVector; + +OVector edges_connecting_to_node(int n) +{ + OVector branch_list_; + for(int i=0;i<n;i++) + branch_list_.push_back(i); + + return branch_list_; +} + +int main(int argc,char* argv[]) +{ + for(int n=0; n < argc; n++) + { + auto x = edges_connecting_to_node(1); + x.clone(); + f(x); + } +}
