Message ID | CAO2gOZXupbCOwLLvjKgUEsV_=aj99AwwdR4x1tKGQD-u_sL9bw@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Mon, Nov 26, 2012 at 11:54 PM, Dehao Chen <dehao@google.com> wrote: > The new patch is attached. Bootstrapped and passed gcc regression test. > > Ok for trunk? Hmm ... this merely avoids some UNKNOWN_LOCATIONs which should be ok. I think the issue is that gimplify_expr does: saved_location = input_location; if (save_expr != error_mark_node && EXPR_HAS_LOCATION (*expr_p)) input_location = EXPR_LOCATION (*expr_p); thus it retains input_location from previous recursive invocations (that's ok). But it doesn't seem to be the case that input_location is UNKNOWN_LOCATION during GIMPLE passes (which it really should be ...). So to fix the gimplification issue I think that gimplify_ctx should have a location (initialized to UNKNOWN_LOCATION), which it saves/restores across its push/pop operation. Or of course that inside the pass manager before executing passes assert that input_location is UNKNOWN_LOCATION and fix up things according to that. First offender is in cgraphunit.c in cgraph_analyze_function: location_t saved_loc = input_location; input_location = DECL_SOURCE_LOCATION (decl); that should be totally unnecessary (input_location shoud be and stay UNKNOWN_LOCATION). And finalize_compilation_unit (the last thing the frontends should call) should have input_location = UNKNOWN_LOCATION right at the point it clears current_function_decl / cfun. I'd prefer the 2nd approach (maybe without the assert as we are in stage3 already, but the assert would certainly help). And places in the middle-end that set input_location for purpose of (re-)gimplifying should use the location in the gimplify ctx (which would need to be added) instead of setting input_location. Maybe as a first try set input_location to UNKNOWN_LOCATION in finalize_compilation_unit. Thanks, Richard. > Thanks, > Dehao > > gcc/ChangeLog: > 2010-11-05 Dehao Chen <dehao@google.com> > > * ipa-prop.c (ipa_modify_call_arguments): Set loc correctly. > * emit-rtl.c (last_location): Remove unused variable. > > Index: gcc/emit-rtl.c > =================================================================== > --- gcc/emit-rtl.c (revision 193203) > +++ gcc/emit-rtl.c (working copy) > @@ -5937,7 +5937,7 @@ location_t epilogue_location; > /* Hold current location information and last location information, so the > datastructures are built lazily only when some instructions in given > place are needed. */ > -static location_t curr_location, last_location; > +static location_t curr_location; > > /* Allocate insn location datastructure. */ > void > @@ -5945,7 +5945,6 @@ insn_locations_init (void) > { > prologue_location = epilogue_location = 0; > curr_location = UNKNOWN_LOCATION; > - last_location = UNKNOWN_LOCATION; > } > > /* At the end of emit stage, clear current location. */ > Index: gcc/ipa-prop.c > =================================================================== > --- gcc/ipa-prop.c (revision 193203) > +++ gcc/ipa-prop.c (working copy) > @@ -2870,7 +2870,8 @@ ipa_modify_call_arguments (struct cgraph_edge *cs, > > gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0); > base = gimple_call_arg (stmt, adj->base_index); > - loc = EXPR_LOCATION (base); > + loc = DECL_P (base) ? DECL_SOURCE_LOCATION (base) > + : EXPR_LOCATION (base); > > if (TREE_CODE (base) != ADDR_EXPR > && POINTER_TYPE_P (TREE_TYPE (base)))
On Tue, Nov 27, 2012 at 1:46 AM, Richard Biener <richard.guenther@gmail.com> wrote: > On Mon, Nov 26, 2012 at 11:54 PM, Dehao Chen <dehao@google.com> wrote: >> The new patch is attached. Bootstrapped and passed gcc regression test. >> >> Ok for trunk? > > Hmm ... this merely avoids some UNKNOWN_LOCATIONs which should > be ok. I think the issue is that gimplify_expr does: > > saved_location = input_location; > if (save_expr != error_mark_node > && EXPR_HAS_LOCATION (*expr_p)) > input_location = EXPR_LOCATION (*expr_p); > > thus it retains input_location from previous recursive invocations (that's ok). > > But it doesn't seem to be the case that input_location is UNKNOWN_LOCATION > during GIMPLE passes (which it really should be ...). So to fix the > gimplification > issue I think that gimplify_ctx should have a location (initialized to > UNKNOWN_LOCATION), which it saves/restores across its push/pop > operation. > > Or of course that inside the pass manager before executing passes assert > that input_location is UNKNOWN_LOCATION and fix up things according > to that. First offender is in cgraphunit.c in cgraph_analyze_function: > > location_t saved_loc = input_location; > input_location = DECL_SOURCE_LOCATION (decl); > > that should be totally unnecessary (input_location shoud be and stay > UNKNOWN_LOCATION). And finalize_compilation_unit (the last thing > the frontends should call) should have input_location = UNKNOWN_LOCATION > right at the point it clears current_function_decl / cfun. > > I'd prefer the 2nd approach (maybe without the assert as we are in stage3 > already, but the assert would certainly help). And places in the middle-end > that set input_location for purpose of (re-)gimplifying should use the location > in the gimplify ctx (which would need to be added) instead of setting > input_location. > > Maybe as a first try set input_location to UNKNOWN_LOCATION in > finalize_compilation_unit. > > Thanks, > Richard. Shall we check in this patch first, and have another patch to reset input_location? Thanks, Dehao
On Tue, Nov 27, 2012 at 3:54 PM, Dehao Chen <dehao@google.com> wrote: > On Tue, Nov 27, 2012 at 1:46 AM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Mon, Nov 26, 2012 at 11:54 PM, Dehao Chen <dehao@google.com> wrote: >>> The new patch is attached. Bootstrapped and passed gcc regression test. >>> >>> Ok for trunk? >> >> Hmm ... this merely avoids some UNKNOWN_LOCATIONs which should >> be ok. I think the issue is that gimplify_expr does: >> >> saved_location = input_location; >> if (save_expr != error_mark_node >> && EXPR_HAS_LOCATION (*expr_p)) >> input_location = EXPR_LOCATION (*expr_p); >> >> thus it retains input_location from previous recursive invocations (that's ok). >> >> But it doesn't seem to be the case that input_location is UNKNOWN_LOCATION >> during GIMPLE passes (which it really should be ...). So to fix the >> gimplification >> issue I think that gimplify_ctx should have a location (initialized to >> UNKNOWN_LOCATION), which it saves/restores across its push/pop >> operation. >> >> Or of course that inside the pass manager before executing passes assert >> that input_location is UNKNOWN_LOCATION and fix up things according >> to that. First offender is in cgraphunit.c in cgraph_analyze_function: >> >> location_t saved_loc = input_location; >> input_location = DECL_SOURCE_LOCATION (decl); >> >> that should be totally unnecessary (input_location shoud be and stay >> UNKNOWN_LOCATION). And finalize_compilation_unit (the last thing >> the frontends should call) should have input_location = UNKNOWN_LOCATION >> right at the point it clears current_function_decl / cfun. >> >> I'd prefer the 2nd approach (maybe without the assert as we are in stage3 >> already, but the assert would certainly help). And places in the middle-end >> that set input_location for purpose of (re-)gimplifying should use the location >> in the gimplify ctx (which would need to be added) instead of setting >> input_location. >> >> Maybe as a first try set input_location to UNKNOWN_LOCATION in >> finalize_compilation_unit. >> >> Thanks, >> Richard. > > Shall we check in this patch first, and have another patch to reset > input_location? Sure, that works for me. It might be harder for you to verify that doing that also fixes the issue though ;) Richard. > Thanks, > Dehao
Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c (revision 193203) +++ gcc/emit-rtl.c (working copy) @@ -5937,7 +5937,7 @@ location_t epilogue_location; /* Hold current location information and last location information, so the datastructures are built lazily only when some instructions in given place are needed. */ -static location_t curr_location, last_location; +static location_t curr_location; /* Allocate insn location datastructure. */ void @@ -5945,7 +5945,6 @@ insn_locations_init (void) { prologue_location = epilogue_location = 0; curr_location = UNKNOWN_LOCATION; - last_location = UNKNOWN_LOCATION; } /* At the end of emit stage, clear current location. */ Index: gcc/ipa-prop.c =================================================================== --- gcc/ipa-prop.c (revision 193203) +++ gcc/ipa-prop.c (working copy) @@ -2870,7 +2870,8 @@ ipa_modify_call_arguments (struct cgraph_edge *cs, gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0); base = gimple_call_arg (stmt, adj->base_index); - loc = EXPR_LOCATION (base); + loc = DECL_P (base) ? DECL_SOURCE_LOCATION (base) + : EXPR_LOCATION (base); if (TREE_CODE (base) != ADDR_EXPR && POINTER_TYPE_P (TREE_TYPE (base)))