Message ID | 518A3D8E.5020800@acm.org |
---|---|
State | New |
Headers | show |
> 2013-05-08 Nathan Sidwell <nathan@codesourcery.com> > > gcc/ > * varasm.c (default_use_anchors_for_symbol_p): Reject WEAK. > > gcc/testsuite/ > * gcc.dg/visibility-21.c: New. > > Index: gcc/varasm.c > =================================================================== > --- gcc/varasm.c (revision 410150) > +++ gcc/varasm.c (working copy) > @@ -6871,6 +6871,11 @@ default_use_anchors_for_symbol_p (const_ > if (!targetm.binds_local_p (decl)) > return false; > > + /* Weak decls might be overridden, but could still be local to > + the module. */ > + if (DECL_WEAK (decl)) > + return false; With LTO we play more of similar tricks, by making use of the resolution file. I.e. for COMMON and EXTERNAL. Does it matter here? I would preffer the renaming excercise, since the name confused me few times, too and the other predicate would be useful for IPA code :) Thanks a lot for looking into this! Honza
On 05/08/13 14:11, Jan Hubicka wrote: > With LTO we play more of similar tricks, by making use of the resolution file. > I.e. for COMMON and EXTERNAL. Does it matter here? probably. It'd be a missed optimization though, rather than wrong code emission. > > I would preffer the renaming excercise, since the name confused me few times, > too and the other predicate would be useful for IPA code :) before invoking sed, can we agree on the right names? I propose * renaming 'binds_local_p' to 'binds_module_p' (and any default implementations etc likewise). Keeps the semantics of returning true if the symbol is known to bind in the dynamic object or executable the current object file ends up in. * a new function or hook 'binds_here_p' (and any default implementations needed). Returns true if the symbol is known to bind in the object file being emitted by the current compilation. 'here's not the best name, but sadly 'object' is overloaded and could mean the object being queried or the object file being emitted. Similarly 'translation unit' is too restrictive in LTO mode, where multiple TUs are in play. I'm open to a better name. Clearly these are related in that binds_here_p returning true implies binds_module_p being true, and binds_module_p returning false implies binds_here_p being false. Because of how gcc's currently structured the implementation of that new hook would be if (binds_local_p () && !WEAK ()) return true; * finally change default_use_anchors_p to use the new hook. thoughts? nathan
Thanks for fixing this. Nathan Sidwell <nathan@acm.org> writes: >> I would preffer the renaming excercise, since the name confused me few times, >> too and the other predicate would be useful for IPA code :) > > before invoking sed, can we agree on the right names? I propose > > * renaming 'binds_local_p' to 'binds_module_p' (and any default implementations > etc likewise). Keeps the semantics of returning true if the symbol is known to > bind in the dynamic object or executable the current object file ends up in. > > * a new function or hook 'binds_here_p' (and any default implementations > needed). Returns true if the symbol is known to bind in the object file being > emitted by the current compilation. > > 'here's not the best name, but sadly 'object' is overloaded and could mean the > object being queried or the object file being emitted. Similarly 'translation > unit' is too restrictive in LTO mode, where multiple TUs are in play. I'm open > to a better name. > > Clearly these are related in that binds_here_p returning true implies > binds_module_p being true, and binds_module_p returning false implies > binds_here_p being false. > > Because of how gcc's currently structured the implementation of that new hook > would be > if (binds_local_p () && !WEAK ()) return true; > > * finally change default_use_anchors_p to use the new hook. > > thoughts? Not really a useful answer, but sometimes if everyone agrees you get no replies, so: sounds great to me FWIW, including the "here" thing. Richard
> On 05/08/13 14:11, Jan Hubicka wrote: > > >With LTO we play more of similar tricks, by making use of the resolution file. > >I.e. for COMMON and EXTERNAL. Does it matter here? > > probably. It'd be a missed optimization though, rather than wrong code emission. Well, the resolution info turns !bind_local_p into binds_local_p knowing the fact that they was bound into non-LTO code from the same module. I think that may also cause anchors to be confused. > > > > >I would preffer the renaming excercise, since the name confused me few times, > >too and the other predicate would be useful for IPA code :) > > before invoking sed, can we agree on the right names? I propose > > * renaming 'binds_local_p' to 'binds_module_p' (and any default > implementations etc likewise). Keeps the semantics of returning > true if the symbol is known to bind in the dynamic object or > executable the current object file ends up in. Sounds good to me. > > * a new function or hook 'binds_here_p' (and any default > implementations needed). Returns true if the symbol is known to > bind in the object file being emitted by the current compilation. Thinking about it again, isn't decl_replaceable_p the thing you are looking for here? > > 'here's not the best name, but sadly 'object' is overloaded and > could mean the object being queried or the object file being > emitted. Similarly 'translation unit' is too restrictive in LTO > mode, where multiple TUs are in play. I'm open to a better name. Hmm, the whole thing is even slightly more complicated, part of it is already handles as part of symtab_availability infrastructure. We basicaly want to ask. 1) will be decl bound to current module? - Now binds_local_p - Proposed binds_module_p - LLVM symbol->isLocalLinkage I think. Useful primarily when generating references (IP relative or GOT/PLT based) 2) will be decl bound to particular definition in current unit - Now decl_replaceable_p perhaps? - Proposed binds_here_p. - LLVM !symbol->isWeakForLinker i think. this is useful for the anchors and also all optimizations that actually needs to hack on the definition. 3) will be decl bound to definition that is equivalnet to one I am seeing? This differs from 1) i.e. for C++ one-decl-rule functions, in particular inlines. - Now cgraph_body_availability (node) >= AVAIL_AVAILABLE for functions const_value_known_p (node) for vairables. plus there is somewhat confusing cgraph_variable_initializer_availability that should be removed. - Proposed ??? - LLVM symbol->mayBeOverridden() Useful for constant propagation, inlining and other stuff. I wonder if we can come with something more clearly named. When it comes to weird predicates on symbols, we also have can_refer_decl_in_current_unit_p. Honza > > Clearly these are related in that binds_here_p returning true > implies binds_module_p being true, and binds_module_p returning > false implies binds_here_p being false. > > Because of how gcc's currently structured the implementation of that > new hook would be > if (binds_local_p () && !WEAK ()) return true; > > * finally change default_use_anchors_p to use the new hook. > > thoughts? > > nathan
On 8 May 2013 15:11:18 Jan Hubicka <hubicka@ucw.cz> wrote: > > 2013-05-08 Nathan Sidwell <nathan@codesourcery.com> > > gcc/ > > * varasm.c (default_use_anchors_for_symbol_p): Reject WEAK. > > gcc/testsuite/ > > * gcc.dg/visibility-21.c: New. > > Index: gcc/varasm.c > > =================================================================== > > --- gcc/varasm.c (revision 410150) > > +++ gcc/varasm.c (working copy) > > @@ -6871,6 +6871,11 @@ default_use_anchors_for_symbol_p (const_ > > if (!targetm.binds_local_p (decl)) > > return false; > > + /* Weak decls might be overridden, but could still be local to > > + the module. */ > > + if (DECL_WEAK (decl)) > > + return false; > > With LTO we play more of similar tricks, by making use of the resolution file. > I.e. for COMMON and EXTERNAL. Does it matter here? > > I would preffer the renaming excercise, since the name confused me few times, > too and the other predicate would be useful for IPA code :) > > Thanks a lot for looking into this! > Honza Does this regress PR32219 ? http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00665.html I don't remember if the patch and test case were applied yet and don't have the sources at hand.. Thanks, Sent with AquaMail for Android http://www.aqua-mail.com
On 2013/5/9 04:11 AM, Bernhard Reutner-Fischer wrote: > On 8 May 2013 15:11:18 Jan Hubicka <hubicka@ucw.cz> wrote: >> > 2013-05-08 Nathan Sidwell <nathan@codesourcery.com> >> > gcc/ >> > * varasm.c (default_use_anchors_for_symbol_p): Reject WEAK. >> > gcc/testsuite/ >> > * gcc.dg/visibility-21.c: New. >> > Index: gcc/varasm.c >> > =================================================================== >> > --- gcc/varasm.c (revision 410150) >> > +++ gcc/varasm.c (working copy) >> > @@ -6871,6 +6871,11 @@ default_use_anchors_for_symbol_p (const_ >> > if (!targetm.binds_local_p (decl)) >> > return false; >> > + /* Weak decls might be overridden, but could still be local to >> > + the module. */ >> > + if (DECL_WEAK (decl)) >> > + return false; >> >> With LTO we play more of similar tricks, by making use of the >> resolution file. >> I.e. for COMMON and EXTERNAL. Does it matter here? >> >> I would preffer the renaming excercise, since the name confused me few >> times, >> too and the other predicate would be useful for IPA code :) >> >> Thanks a lot for looking into this! >> Honza > > Does this regress PR32219 ? > http://gcc.gnu.org/ml/gcc-patches/2010-03/msg00665.html > I don't remember if the patch and test case were applied yet and don't > have the sources at hand.. > > Thanks, I don't think so (Nathan and I were also discussing that PR in the context of this fix). The testcase in that PR still segfaults, which I have another fix. Will post later. Chung-Lin
On 05/08/13 18:47, Jan Hubicka wrote: >> On 05/08/13 14:11, Jan Hubicka wrote: >> * a new function or hook 'binds_here_p' (and any default >> implementations needed). Returns true if the symbol is known to >> bind in the object file being emitted by the current compilation. > > Thinking about it again, isn't decl_replaceable_p the thing you are looking for > here? that looks promising. I'll try !decl_replaceable_p in the section anchor hook. > I wonder if we can come with something more clearly named. > When it comes to weird predicates on symbols, we also have > can_refer_decl_in_current_unit_p. thanks for your comments, if decl_replaceable_p is the right existing predicate, that does make figuring out the right set of names less urgent. nathan
Index: gcc/varasm.c =================================================================== --- gcc/varasm.c (revision 410150) +++ gcc/varasm.c (working copy) @@ -6871,6 +6871,11 @@ default_use_anchors_for_symbol_p (const_ if (!targetm.binds_local_p (decl)) return false; + /* Weak decls might be overridden, but could still be local to + the module. */ + if (DECL_WEAK (decl)) + return false; + /* Don't use section anchors for decls that will be placed in a small data section. */ /* ??? Ideally, this check would be redundant with the SECTION_SMALL Index: gcc/testsuite/gcc.dg/visibility-21.c =================================================================== --- gcc/testsuite/gcc.dg/visibility-21.c (revision 0) +++ gcc/testsuite/gcc.dg/visibility-21.c (revision 0) @@ -0,0 +1,13 @@ +/* Test visibility attribute on function definition. */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fsection-anchors" } */ +/* { dg-require-visibility "" } */ +/* { dg-require-weak "" } */ +/* { dg-final { scan-assembler-not "ANCHOR" } } */ + +int __attribute__((weak, visibility("hidden"))) weak_hidden[3]; + +int *f_weak_hidden () +{ + return weak_hidden; +}