| Message ID | 56758A2A.4090206@acm.org |
|---|---|
| State | New |
| Headers | show |
On 12/19/2015 09:47 AM, Nathan Sidwell wrote: > > Either build_constant_desc (varasm.c) should put constants into the > varpool or decl_in_symtab_p should ignore constant pool VAR_DECLS. It > appears to me that the latter is the right choice, as constant pool > entries can't be related to regular varpool entries (right?) > > Thus this patch augments the decl_in_symtab_p function to ignore > constant pool entries. While debugging I found the logic in > compare_base_decls of 'in_symtab1 != in_symtab2 || !in_symtab1' to be > rather confusing, and simplified it to be (the moral equivalent of) > '!in_symtab1 || !in_symtab2'. > > This looks target-agnostic, and manifests on (at least) x86_64-linux too > -- except most targets don't need to emit declarations of undefined > externs, so they don't explode. The only other target I'm aware of where this is an issue is the PA. And it may only be an issue in the older 32-bit SOM runtime. Essentially you have to "import" everything you use from other TUs and bad things happen if you "import" something that is actually file scoped in the current TU. > > > ok? With some kind of comment in decl_in_symtab_p indicating why we need to check and filter on !DECL_IN_CONSTANT_POOL this is OK. jeff
2015-12-19 Nathan Sidwell <nathan@acm.org> gcc/ * alias.c (compare_base_decls): Simplify in-symtab check. * cgraph.h (decl_in_symtab_p): Check DECL_IN_CONSTANT_POOL. gcc/testsuite/ * gcc.dg/alias-15.c: New. Index: alias.c =================================================================== --- alias.c (revision 231841) +++ alias.c (working copy) @@ -2038,13 +2038,12 @@ compare_base_decls (tree base1, tree bas if (base1 == base2) return 1; - bool in_symtab1 = decl_in_symtab_p (base1); - bool in_symtab2 = decl_in_symtab_p (base2); - /* Declarations of non-automatic variables may have aliases. All other decls are unique. */ - if (in_symtab1 != in_symtab2 || !in_symtab1) + if (!decl_in_symtab_p (base1) + || !decl_in_symtab_p (base2)) return 0; + ret = symtab_node::get_create (base1)->equal_address_to (symtab_node::get_create (base2), true); if (ret == 2) Index: cgraph.h =================================================================== --- cgraph.h (revision 231841) +++ cgraph.h (working copy) @@ -2301,6 +2301,7 @@ decl_in_symtab_p (const_tree decl) { return (TREE_CODE (decl) == FUNCTION_DECL || (TREE_CODE (decl) == VAR_DECL + && !DECL_IN_CONSTANT_POOL (decl) && (TREE_STATIC (decl) || DECL_EXTERNAL (decl)))); } Index: testsuite/gcc.dg/alias-15.c =================================================================== --- testsuite/gcc.dg/alias-15.c (revision 0) +++ testsuite/gcc.dg/alias-15.c (working copy) @@ -0,0 +1,15 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-O2 -fdump-ipa-cgraph" } */ + +/* RTL-level CSE shouldn't introduce LCO (for the string) into varpool */ +char *p; + +void foo () +{ + p = "abc\n"; + + while (*p != '\n') + p++; +} + +/* { dg-final { scan-ipa-dump-not "LC0" "cgraph" } } */
A recent PTX patch of mine exposed a latent bug in varpool handling by exposing more CSE opportunities. In the attached testcase: char *p; void foo () { p = "abc\n"; while (*p != '\n') p++; } the RTL CSE pass checks whether SYMBOL_REF (p) and SYMBOL_REF ($LCO) are related. LC0 is the constant pool entry for the string constant. In doing this, we end up in compare_base_decls (alias.c), and decl_in_symtab_p thinks '$LC0' should be in the symtab (it's a TREE_STATIC VAR_DECL). Consequently we continue on to symtab_node::get_create (base2), and create a symtab entry for $LCO. That entry thinks $LCO is defined in another TU. (this is the point at which PTX blows up because $LCO is emitted to the assembly file as both a static definition and an extern declaration.) Either build_constant_desc (varasm.c) should put constants into the varpool or decl_in_symtab_p should ignore constant pool VAR_DECLS. It appears to me that the latter is the right choice, as constant pool entries can't be related to regular varpool entries (right?) Thus this patch augments the decl_in_symtab_p function to ignore constant pool entries. While debugging I found the logic in compare_base_decls of 'in_symtab1 != in_symtab2 || !in_symtab1' to be rather confusing, and simplified it to be (the moral equivalent of) '!in_symtab1 || !in_symtab2'. This looks target-agnostic, and manifests on (at least) x86_64-linux too -- except most targets don't need to emit declarations of undefined externs, so they don't explode. Without the patch, the attached testcase shows an additional varpool entry of: *.LC0/2 (*.LC0) @0x7fa5c7d3a400 Type: variable Visibility: asm_written artificial References: Referring: Availability: not_available Varpool flags: initialized read-only const-value-known ok? nathan