Message ID | 20131214220153.GA18087@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
Hi, On Sat, Dec 14, 2013 at 11:01:53PM +0100, Jan Hubicka wrote: > Hi, > this patch makes stmt_may_be_vtbl_ptr_store to skip clobbers as discussed > previously. This is the first time I hear about this but the change is obviously OK, thanks. > Martin, I do not fully follow the logic of this function. Can't we just > ignore all stores that are not of pointer type that looks like vptr > type? All those tores are compiler generated and we probably can just > annotate them, right? Richard wanted me to be quite conservative here and to a big extent I can see his point. The AGGREGATE_TYPE_P is there because copies of the whole object could change the vptr in an unpredictable ways. I doubt that this is what copy-constructors get expanded to but gimple does not provide any necessary guarantees. Perhaps we could come up with an elaborate explanation why it cannot happen in valid C++ just as I did for the call statement (and that is written in the comment above the function) and then we could remove it. Of course, having gimple defined and required annotation for vptr changes would be much better but then of course all transformations would have to make sure they preserve it. IIRC, flag_strict_aliasing test was explicitely Richard's request, perhaps we could restrict the pointer type test further. Does that answer your question? Martin > > Bootstrapped/regtested x86_64-linux, comitted. > > Honza > > * ipa-prop.c (stmt_may_be_vtbl_ptr_store): Skip clobbers. > * g++.dg/ipa/devirt-19.C: New testcase. > Index: ipa-prop.c > =================================================================== > --- ipa-prop.c (revision 205987) > +++ ipa-prop.c (working copy) > @@ -560,6 +560,8 @@ stmt_may_be_vtbl_ptr_store (gimple stmt) > { > if (is_gimple_call (stmt)) > return false; > + else if (gimple_clobber_p (stmt)) > + return false; > else if (is_gimple_assign (stmt)) > { > tree lhs = gimple_assign_lhs (stmt); > Index: testsuite/g++.dg/ipa/devirt-19.C > =================================================================== > --- testsuite/g++.dg/ipa/devirt-19.C (revision 0) > +++ testsuite/g++.dg/ipa/devirt-19.C (revision 0) > @@ -0,0 +1,32 @@ > +/* We should specialize for &b and devirtualize the call. > + Previously we were failing by considering CLOBBER statement to be > + a type change. */ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fdump-ipa-cp" } */ > +struct A { > + void operator==(const A &); > +}; > +class B { > +public: > + A m_fn1(); > + A m_fn2(); > +}; > +template <typename T, typename M> class C { > +public: > + T Key; > + const M &m_fn2(const T &); > + virtual void m_fn1() {} > + B _map; > +}; > + > +C<int, int> b; > +template <typename T, typename M> const M &C<T, M>::m_fn2(const T &) { > + A a = _map.m_fn2(); > + a == _map.m_fn1(); > + m_fn1(); > +} > + > +void fn1() { b.m_fn2(0); } > +/* { dg-final { scan-ipa-dump-times "Discovered a virtual call to a known target" 1 "cp" } } */ > +/* { dg-final { cleanup-ipa-dump "cp" } } */ > +
> Hi, > > On Sat, Dec 14, 2013 at 11:01:53PM +0100, Jan Hubicka wrote: > > Hi, > > this patch makes stmt_may_be_vtbl_ptr_store to skip clobbers as discussed > > previously. > > This is the first time I hear about this but the change is obviously > OK, thanks. It actually seems to solve quite number of false positives, not exactly sure why. > > > Martin, I do not fully follow the logic of this function. Can't we just > > ignore all stores that are not of pointer type that looks like vptr > > type? All those tores are compiler generated and we probably can just > > annotate them, right? > > Richard wanted me to be quite conservative here and to a big extent I > can see his point. The AGGREGATE_TYPE_P is there because copies of > the whole object could change the vptr in an unpredictable ways. I > doubt that this is what copy-constructors get expanded to but gimple > does not provide any necessary guarantees. Perhaps we could come up > with an elaborate explanation why it cannot happen in valid C++ just > as I did for the call statement (and that is written in the comment > above the function) and then we could remove it. OK, so the explanation is not as simple as claim that non-POD types needs to be constructed or copied by constructor and C++ FE always generate an explicit vtbl store? > > Of course, having gimple defined and required annotation for vptr > changes would be much better but then of course all transformations > would have to make sure they preserve it. > > IIRC, flag_strict_aliasing test was explicitely Richard's request, > perhaps we could restrict the pointer type test further. > > Does that answer your question? Sort of, yes. We should make some analysis how effective current methods are (i.e. disabling it and checking how much devirtualization improve for firefox) and if we find they seem insufficient, we probably should think of better analysis or annotation... Honza
On Mon, Dec 16, 2013 at 2:58 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> Hi, >> >> On Sat, Dec 14, 2013 at 11:01:53PM +0100, Jan Hubicka wrote: >> > Hi, >> > this patch makes stmt_may_be_vtbl_ptr_store to skip clobbers as discussed >> > previously. >> >> This is the first time I hear about this but the change is obviously >> OK, thanks. > > It actually seems to solve quite number of false positives, not exactly sure why. >> >> > Martin, I do not fully follow the logic of this function. Can't we just >> > ignore all stores that are not of pointer type that looks like vptr >> > type? All those tores are compiler generated and we probably can just >> > annotate them, right? >> >> Richard wanted me to be quite conservative here and to a big extent I >> can see his point. The AGGREGATE_TYPE_P is there because copies of >> the whole object could change the vptr in an unpredictable ways. I >> doubt that this is what copy-constructors get expanded to but gimple >> does not provide any necessary guarantees. Perhaps we could come up >> with an elaborate explanation why it cannot happen in valid C++ just >> as I did for the call statement (and that is written in the comment >> above the function) and then we could remove it. > > OK, so the explanation is not as simple as claim that non-POD types > needs to be constructed or copied by constructor and C++ FE always > generate an explicit vtbl store? No as optimizers may combine stores for example. >> Of course, having gimple defined and required annotation for vptr >> changes would be much better but then of course all transformations >> would have to make sure they preserve it. >> >> IIRC, flag_strict_aliasing test was explicitely Richard's request, >> perhaps we could restrict the pointer type test further. >> >> Does that answer your question? > > Sort of, yes. We should make some analysis how effective current > methods are (i.e. disabling it and checking how much devirtualization > improve for firefox) and if we find they seem insufficient, we probably > should think of better analysis or annotation... > > Honza
> > OK, so the explanation is not as simple as claim that non-POD types > > needs to be constructed or copied by constructor and C++ FE always > > generate an explicit vtbl store? > > No as optimizers may combine stores for example. Yep, I understand we can design "evil" optimization that will make problem of tracking virtual table pointers very hard. The question is how much we want to have these pre-IPA and how their benefits relate to beenfits from improved devirtualization. We are still weak on deivrt (and current generation benchmarks don't seem to care much on how good we are), so at the moment my approach is to improve the analysis without imposing additional restriction. but in longer turn we may want to make some compromises (pre-ipa) here and preserve more information in gimple. Honza > > >> Of course, having gimple defined and required annotation for vptr > >> changes would be much better but then of course all transformations > >> would have to make sure they preserve it. > >> > >> IIRC, flag_strict_aliasing test was explicitely Richard's request, > >> perhaps we could restrict the pointer type test further. > >> > >> Does that answer your question? > > > > Sort of, yes. We should make some analysis how effective current > > methods are (i.e. disabling it and checking how much devirtualization > > improve for firefox) and if we find they seem insufficient, we probably > > should think of better analysis or annotation... > > > > Honza
On Wed, Dec 18, 2013 at 1:39 PM, Jan Hubicka <hubicka@ucw.cz> wrote: >> > OK, so the explanation is not as simple as claim that non-POD types >> > needs to be constructed or copied by constructor and C++ FE always >> > generate an explicit vtbl store? >> >> No as optimizers may combine stores for example. > > Yep, I understand we can design "evil" optimization that will make problem > of tracking virtual table pointers very hard. > The question is how much we want to have these pre-IPA and how their benefits > relate to beenfits from improved devirtualization. Well, if devirt relies on code appearing in a very special way and only in that way then it is broken. devirt, as any other transform, has to work conservative correctly. As a way out you can define a unique way how vtable pointer init has to work and that other passes don't mess with (because they don't know it). For example add a no-op handled component VTBL_REF (or not no-op and encode an offset with it). > We are still weak on deivrt (and current generation benchmarks don't seem to > care much on how good we are), so at the moment my approach is to improve the > analysis without imposing additional restriction. but in longer turn we may > want to make some compromises (pre-ipa) here and preserve more information in gimple. At the moment GIMPLE doesn't know about vtable pointers or the vtable slot. You'd have to teach it in some way and at some point lower. Richard. > Honza >> >> >> Of course, having gimple defined and required annotation for vptr >> >> changes would be much better but then of course all transformations >> >> would have to make sure they preserve it. >> >> >> >> IIRC, flag_strict_aliasing test was explicitely Richard's request, >> >> perhaps we could restrict the pointer type test further. >> >> >> >> Does that answer your question? >> > >> > Sort of, yes. We should make some analysis how effective current >> > methods are (i.e. disabling it and checking how much devirtualization >> > improve for firefox) and if we find they seem insufficient, we probably >> > should think of better analysis or annotation... >> > >> > Honza
Index: ipa-prop.c =================================================================== --- ipa-prop.c (revision 205987) +++ ipa-prop.c (working copy) @@ -560,6 +560,8 @@ stmt_may_be_vtbl_ptr_store (gimple stmt) { if (is_gimple_call (stmt)) return false; + else if (gimple_clobber_p (stmt)) + return false; else if (is_gimple_assign (stmt)) { tree lhs = gimple_assign_lhs (stmt); Index: testsuite/g++.dg/ipa/devirt-19.C =================================================================== --- testsuite/g++.dg/ipa/devirt-19.C (revision 0) +++ testsuite/g++.dg/ipa/devirt-19.C (revision 0) @@ -0,0 +1,32 @@ +/* We should specialize for &b and devirtualize the call. + Previously we were failing by considering CLOBBER statement to be + a type change. */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fdump-ipa-cp" } */ +struct A { + void operator==(const A &); +}; +class B { +public: + A m_fn1(); + A m_fn2(); +}; +template <typename T, typename M> class C { +public: + T Key; + const M &m_fn2(const T &); + virtual void m_fn1() {} + B _map; +}; + +C<int, int> b; +template <typename T, typename M> const M &C<T, M>::m_fn2(const T &) { + A a = _map.m_fn2(); + a == _map.m_fn1(); + m_fn1(); +} + +void fn1() { b.m_fn2(0); } +/* { dg-final { scan-ipa-dump-times "Discovered a virtual call to a known target" 1 "cp" } } */ +/* { dg-final { cleanup-ipa-dump "cp" } } */ +