| Message ID | 55A28D67.7010001@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Sun, Jul 12, 2015 at 5:53 PM, Tom de Vries <Tom_deVries@mentor.com> wrote: > On 12/07/15 17:45, Tom de Vries wrote: >> >> Hi, >> >> this patch series implements the forbidding of multi-step garbage >> collection liveness dependencies between caches. >> >> The first four patches downgrade 3 caches to non-cache, since they >> introduce multi-step dependencies. This allows us to decouple: >> - establishing a policy for multi-step dependencies in caches, and >> - fixing issues that allow us to use these 3 as caches again. >> >> 1. Downgrade debug_args_for_decl to non-cache >> 2. Add struct tree_decl_map_hasher >> 3. Downgrade debug_expr_for_decl to non-cache >> 4. Downgrade value_expr_for_decl to non-cache >> 5. Don't mark live recursively in gt_cleare_cache >> >> Bootstrapped and reg-tested on x86_64, with ENABLE_CHECKING. >> >> I'll post the patches in response to this email. > > > This patch downgrade value_expr_for_decl to non-cache. > > OK for trunk? Similar to the debug_decl_map the idea of this hash is that it records on-the-side info for an entity. If that entity is no longer needed then we should discard the reference so we can collect the entity. Whether the on-the-side info is used in other means isn't relevant here. Note that they are also not really "caches" in that removing an entry for a key that is still live will cause information loss that cannot be restored. What's the bad side-effect of the "late" marking other than affecting this (or other) caches? What's your idea of restoring the "idea" of these maps under the constraint you try to add? Richard. > Thanks, > - Tom >
On Tue, Jul 14, 2015 at 12:45 PM, Richard Biener <richard.guenther@gmail.com> wrote: > On Sun, Jul 12, 2015 at 5:53 PM, Tom de Vries <Tom_deVries@mentor.com> wrote: >> On 12/07/15 17:45, Tom de Vries wrote: >>> >>> Hi, >>> >>> this patch series implements the forbidding of multi-step garbage >>> collection liveness dependencies between caches. >>> >>> The first four patches downgrade 3 caches to non-cache, since they >>> introduce multi-step dependencies. This allows us to decouple: >>> - establishing a policy for multi-step dependencies in caches, and >>> - fixing issues that allow us to use these 3 as caches again. >>> >>> 1. Downgrade debug_args_for_decl to non-cache >>> 2. Add struct tree_decl_map_hasher >>> 3. Downgrade debug_expr_for_decl to non-cache >>> 4. Downgrade value_expr_for_decl to non-cache >>> 5. Don't mark live recursively in gt_cleare_cache >>> >>> Bootstrapped and reg-tested on x86_64, with ENABLE_CHECKING. >>> >>> I'll post the patches in response to this email. >> >> >> This patch downgrade value_expr_for_decl to non-cache. >> >> OK for trunk? > > Similar to the debug_decl_map the idea of this hash is that > it records on-the-side info for an entity. If that entity is no > longer needed then we should discard the reference so we > can collect the entity. Whether the on-the-side info is used > in other means isn't relevant here. > > Note that they are also not really "caches" in that removing > an entry for a key that is still live will cause information loss > that cannot be restored. > > What's the bad side-effect of the "late" marking other than > affecting this (or other) caches? > > What's your idea of restoring the "idea" of these maps under > the constraint you try to add? For example have those special caches have two marking phases. The first phase marks all non-key edges originating from each entry. The second phase is the same as what we have now - unmarked entries get removed. The first phase would go with regular marking, the second when processing caches. You'd delay collecting the memory the non-key edges point to to the next GC run, but I think that's a fair trade-off. Richard. > Richard. > >> Thanks, >> - Tom >>
Hi, On Tue, 14 Jul 2015, Richard Biener wrote: > For example have those special caches have two marking phases. The first > phase marks all non-key edges originating from each entry. The second > phase is the same as what we have now - unmarked entries get removed. > > The first phase would go with regular marking, the second when > processing caches. > > You'd delay collecting the memory the non-key edges point to > to the next GC run, but I think that's a fair trade-off. That's Toms other approach with supporting multi-step dependencies. As I have tried to argue in the other thread, I think this idea is fundamentally broken and just hides real bugs, and I don't see why this would be different for this particular hash-map. If the value of this hash refers to a decl that isn't mentioned anywhere else except from this hash entry, then it has no meaning anymore, and hence shouldn't itself be part of the hash anymore. Ciao, Michael.
On Wed, Jul 15, 2015 at 04:14:07PM +0200, Michael Matz wrote: > That's Toms other approach with supporting multi-step dependencies. As I > have tried to argue in the other thread, I think this idea is > fundamentally broken and just hides real bugs, and I don't see why this > would be different for this particular hash-map. If the value of this > hash refers to a decl that isn't mentioned anywhere else except from this > hash entry, then it has no meaning anymore, and hence shouldn't itself be > part of the hash anymore. You mean key or value? The value of course can mention various trees that aren't referenced from anywhere else, and it has meaning. Jakub
Hi, On Wed, 15 Jul 2015, Jakub Jelinek wrote: > On Wed, Jul 15, 2015 at 04:14:07PM +0200, Michael Matz wrote: > > That's Toms other approach with supporting multi-step dependencies. As I > > have tried to argue in the other thread, I think this idea is > > fundamentally broken and just hides real bugs, and I don't see why this > > would be different for this particular hash-map. If the value of this > > hash refers to a decl that isn't mentioned anywhere else except from this > > hash entry, then it has no meaning anymore, and hence shouldn't itself be > > part of the hash anymore. > > You mean key or value? The value of course can mention various trees that > aren't referenced from anywhere else, and it has meaning. No, I really meant value. If you think it has meaning, then tell me what it is for DECL_VALUE_EXPR (X) to be 'Y', if Y is nowhere else mentioned, neither in code, nor in local-decls, nor in globals, or anywhere else that would be reachable by GC. Ciao, Michael.
On Wed, Jul 15, 2015 at 04:25:44PM +0200, Michael Matz wrote: > On Wed, 15 Jul 2015, Jakub Jelinek wrote: > > > On Wed, Jul 15, 2015 at 04:14:07PM +0200, Michael Matz wrote: > > > That's Toms other approach with supporting multi-step dependencies. As I > > > have tried to argue in the other thread, I think this idea is > > > fundamentally broken and just hides real bugs, and I don't see why this > > > would be different for this particular hash-map. If the value of this > > > hash refers to a decl that isn't mentioned anywhere else except from this > > > hash entry, then it has no meaning anymore, and hence shouldn't itself be > > > part of the hash anymore. > > > > You mean key or value? The value of course can mention various trees that > > aren't referenced from anywhere else, and it has meaning. > > No, I really meant value. If you think it has meaning, then tell me what > it is for DECL_VALUE_EXPR (X) to be 'Y', if Y is nowhere else mentioned, > neither in code, nor in local-decls, nor in globals, or anywhere else that > would be reachable by GC. Pretty much anything, DECL_VALUE_EXPR (X) is some expression. It can be some_var[some_other_var], *some_var, ptr->foo, etc. just to list a few of the ones currently in use. DECL_DEBUG_EXPR can also be __imag__ somevar, __real__ somevar, something.field, etc. Jakub
Hi, On Wed, 15 Jul 2015, Jakub Jelinek wrote: > > No, I really meant value. If you think it has meaning, then tell me > > what it is for DECL_VALUE_EXPR (X) to be 'Y', if Y is nowhere else > > mentioned, neither in code, nor in local-decls, nor in globals, or > > anywhere else that would be reachable by GC. > > Pretty much anything, DECL_VALUE_EXPR (X) is some expression. > It can be some_var[some_other_var], *some_var, ptr->foo, etc. > just to list a few of the ones currently in use. Yes, I know all that. I haven't made myself clear, if 'Y' above is a decl, and it's mentioned nowhere else (so it has no place and has no value), then what meaning could possibly be given to DECL_VALUE_EXPR(X) if it were 'Y'? Similar for "ptr->foo" if "ptr" is nowhere mentioned in code or tables. In effect DECL_VALUE_EXPR refers to stale decls that aren't initialized, aren't given a place and aren't dealt with in code. > DECL_DEBUG_EXPR can also be __imag__ somevar, __real__ somevar, > something.field, etc. Sure, and the same applies, if "something" is a stale decl, then what's the meaning of "something.field" in DECL_DEBUG_EXPR? Ciao, Michael.
Hi, On Wed, 15 Jul 2015, Michael Matz wrote: > Similar for "ptr->foo" if "ptr" is nowhere mentioned in code or tables. > In effect DECL_VALUE_EXPR refers to stale decls that aren't initialized, > aren't given a place and aren't dealt with in code. Or, maybe we're talking past each other. You mean the case where complicated-expr-on-Y is the value-expr, and Y is _no_ stale decl, but the complicated expr itself nevertheless is mentioned nowhere else? Yes, those trees must be retained, I was only talking about the stale-decl cases. Ciao, Michael.
On July 15, 2015 4:52:41 PM GMT+02:00, Michael Matz <matz@suse.de> wrote: >Hi, > >On Wed, 15 Jul 2015, Michael Matz wrote: > >> Similar for "ptr->foo" if "ptr" is nowhere mentioned in code or >tables. >> In effect DECL_VALUE_EXPR refers to stale decls that aren't >initialized, >> aren't given a place and aren't dealt with in code. > >Or, maybe we're talking past each other. You mean the case where >complicated-expr-on-Y is the value-expr, and Y is _no_ stale decl, but >the >complicated expr itself nevertheless is mentioned nowhere else? Yes, >those trees must be retained, I was only talking about the stale-decl >cases. Yes. And that's the case where we still want to collect the decl if it is not mentioned anywhere else. Thus use a cache map. But then we'll ICE on the new sanity check. > >Ciao, >Michael.
On Wed, Jul 15, 2015 at 04:52:41PM +0200, Michael Matz wrote: > On Wed, 15 Jul 2015, Michael Matz wrote: > > > Similar for "ptr->foo" if "ptr" is nowhere mentioned in code or tables. > > In effect DECL_VALUE_EXPR refers to stale decls that aren't initialized, > > aren't given a place and aren't dealt with in code. > > Or, maybe we're talking past each other. You mean the case where > complicated-expr-on-Y is the value-expr, and Y is _no_ stale decl, but the > complicated expr itself nevertheless is mentioned nowhere else? Yes, > those trees must be retained, I was only talking about the stale-decl > cases. I meant primarly all those ADDR_EXPRs, MEM_REFs, INDIRECT_REFs, ARRAY_REFs, etc. For referenced decls in there if they aren't referenced from anywhere else, not 100% sure about it. DECL_VALUE_EXPR is mainly used during gimplification, and then for debug info production, DECL_DEBUG_EXPR too. For debug info, I bet if the underlying decls referenced in there will not exist, then we'd just give up in debug info generation of the particular object, but wonder if gcc won't ICE if you have the flag bits like DECL_HAS_VALUE_EXPR_P set and then DECL_VALUE_EXPR of NULL, if you'd GC them. Another option for GC these might be to gc walk DECL_VALUE_EXPR of decls that have DECL_HAS_VALUE_EXPR_P set, similarly for DECL_DEBUG_EXPR, and then treat those hash maps as pure caches, entries with unmarked keys would be removed, and no walking of trees referenced from the hash map would be performed directly. Jakub
Hi, On Wed, 15 Jul 2015, Richard Biener wrote: > >Or, maybe we're talking past each other. You mean the case where > >complicated-expr-on-Y is the value-expr, and Y is _no_ stale decl, but > >the complicated expr itself nevertheless is mentioned nowhere else? > >Yes, those trees must be retained, I was only talking about the > >stale-decl cases. > > Yes. And that's the case where we still want to collect the decl if it > is not mentioned anywhere else. Thus use a cache map. But then we'll > ICE on the new sanity check. Like Jakubs mentioned, that can (and probably should) better be fixed by walking DECL_VALUE_EXPR from the decl walker, as if it were a normal member (likewise for the other maps, if necessary). It's not necessary to fiddle with the hash-table markers itself. Ciao, Michael.
[PATCH 4/5] Downgrade value_expr_for_decl to non-cache Without this patch, but with patch "Don't mark live recursively in gt_cleare_cache" when compiling soft-fp/divtf3.c -m32 we run into: ... 0x133f37e void gt_cleare_cache<tree_decl_map_cache_hasher>(hash_table<tree_decl_map_cache_hasher, xcallocator>*) /home/vries/gcc_versions/devel/src/gcc/hash-table.h:1114 0x133bfdb gt_clear_caches_gt_tree_h() ./gt-tree.h:475 0x6c8d0f gt_clear_caches() ./gtype-c.h:151 0xa8ef1e ggc_mark_roots() /home/vries/gcc_versions/devel/src/gcc/ggc-common.c:103 0x7f7bfe ggc_collect() /home/vries/gcc_versions/devel/src/gcc/ggc-page.c:2183 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <http://gcc.gnu.org/bugs.html> for instructions. ... The offending cache entry is: ... (gdb) call debug_generic_expr ( (*iter).base.from ) <retval> (gdb) call debug_generic_expr ( (*iter).to ) *.result_ptr ... 2015-07-10 Tom de Vries <tom@codesourcery.com> PR libgomp/66714 * tree.c (value_expr_for_decl): Use tree_decl_map_hasher instead of tree_decl_map_cache_hasher. Don't use cache GTY attribute. (init_ttree): Allocate value_expr_for_decl using new type. --- gcc/tree.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gcc/tree.c b/gcc/tree.c index 6038fff..bb4467d 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -253,8 +253,13 @@ static GTY ((cache)) static GTY (()) hash_table<tree_decl_map_hasher> *debug_expr_for_decl; +/* TODO: Figure out whether we can declare value_expr_for_decl as: static GTY ((cache)) hash_table<tree_decl_map_cache_hasher> *value_expr_for_decl; +*/ + +static GTY (()) + hash_table<tree_decl_map_hasher> *value_expr_for_decl; struct tree_vec_map_cache_hasher : ggc_cache_ptr_hash<tree_vec_map> { @@ -667,7 +672,7 @@ init_ttree (void) = hash_table<tree_decl_map_hasher>::create_ggc (512); value_expr_for_decl - = hash_table<tree_decl_map_cache_hasher>::create_ggc (512); + = hash_table<tree_decl_map_hasher>::create_ggc (512); int_cst_hash_table = hash_table<int_cst_hasher>::create_ggc (1024); -- 1.9.1