* Re: spec2000 regression @ 2001-08-15 5:57 Richard Kenner 2001-08-15 6:03 ` Jan Hubicka 0 siblings, 1 reply; 24+ messages in thread From: Richard Kenner @ 2001-08-15 5:57 UTC (permalink / raw) To: jh; +Cc: gcc-patches the problem is that actually code first sets the DECL_RTL and then calls set_mem_attributes, that calls get_alias_set, but it thinks that the alias set is already computed, but it isn't. I am now testing following patch. OK to install if it succeeds? I'd prefer to fix things so they don't set DECL_RTL first. MEM_ALIAS_SET is not a test to see if the alias set has been set. The alias set might have been zero in which case we *do* want to use it. So I think your change is wrong. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 5:57 spec2000 regression Richard Kenner @ 2001-08-15 6:03 ` Jan Hubicka 2001-08-15 6:36 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Jan Hubicka @ 2001-08-15 6:03 UTC (permalink / raw) To: Richard Kenner; +Cc: jh, gcc-patches > the problem is that actually code first sets the DECL_RTL and then > calls set_mem_attributes, that calls get_alias_set, but it thinks that > the alias set is already computed, but it isn't. > > I am now testing following patch. OK to install if it succeeds? > > I'd prefer to fix things so they don't set DECL_RTL first. MEM_ALIAS_SET > is not a test to see if the alias set has been set. The alias set might > have been zero in which case we *do* want to use it. That what I was trying to do first, but there are number of places this is happening and fixing each of it still keeps open door for possible new bugs - it is natural to save the RTL to proper place first. If I understand the purpose of original patch properly, it was hooting for case variable gets unique class, but this class is recomputed several times so it get several unique classes. For class 0 this does not matter. Honza > > So I think your change is wrong. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 6:03 ` Jan Hubicka @ 2001-08-15 6:36 ` Jason Merrill 2001-08-15 6:52 ` Jan Hubicka 0 siblings, 1 reply; 24+ messages in thread From: Jason Merrill @ 2001-08-15 6:36 UTC (permalink / raw) To: Jan Hubicka; +Cc: Richard Kenner, gcc-patches >>>>> "Jan" == Jan Hubicka <jh@suse.cz> writes: >> I'd prefer to fix things so they don't set DECL_RTL first. MEM_ALIAS_SET >> is not a test to see if the alias set has been set. The alias set might >> have been zero in which case we *do* want to use it. > That what I was trying to do first, but there are number of places this > is happening and fixing each of it still keeps open door for possible > new bugs - it is natural to save the RTL to proper place first. I disagree. Even without this bug, it's bad style. > If I understand the purpose of original patch properly, it was hooting for > case variable gets unique class, but this class is recomputed several times > so it get several unique classes. > For class 0 this does not matter. On the contrary, it is needed for the case where a variable gets a class different from that of its type, so recomputing it would give the wrong answer. How does this work for you? 2001-08-15 Jason Merrill <jason_merrill@redhat.com> * varasm.c (make_decl_rtl): Don't SET_DECL_RTL until we're done modifying the rtl. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 6:36 ` Jason Merrill @ 2001-08-15 6:52 ` Jan Hubicka 2001-08-15 7:11 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Jan Hubicka @ 2001-08-15 6:52 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, Richard Kenner, gcc-patches bad style or not, I believe it would be somewhat dificult to avoid getting it into gcc. Such bugs can be dificult to find, as they waste only the performance. Thanks Andreas for his automated tester :) > > On the contrary, it is needed for the case where a variable gets a class > different from that of its type, so recomputing it would give the wrong > answer. How does this work for you? > > 2001-08-15 Jason Merrill <jason_merrill@redhat.com> > > * varasm.c (make_decl_rtl): Don't SET_DECL_RTL until we're done > modifying the rtl. > Yes, I've made the same patch before the one I sent to list and it solves this particular problem. Note that function.c contains other similar code - just take a look for set_memory_attributes calls contains DECL_RTL in the argument. Honza ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 6:52 ` Jan Hubicka @ 2001-08-15 7:11 ` Jason Merrill 2001-08-15 7:13 ` Jan Hubicka ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Jason Merrill @ 2001-08-15 7:11 UTC (permalink / raw) To: Jan Hubicka; +Cc: Richard Kenner, gcc-patches This should avoid such problems. 2001-08-15 Jason Merrill <jason_merrill@redhat.com> * explow.c (set_mem_attributes): Avoid returning a bogus alias set from a new MEM. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 7:11 ` Jason Merrill @ 2001-08-15 7:13 ` Jan Hubicka 2001-08-15 7:36 ` Jan Hubicka 2001-08-15 8:04 ` Mark Mitchell 2 siblings, 0 replies; 24+ messages in thread From: Jan Hubicka @ 2001-08-15 7:13 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, Richard Kenner, gcc-patches > This should avoid such problems. > > 2001-08-15 Jason Merrill <jason_merrill@redhat.com> > > * explow.c (set_mem_attributes): Avoid returning a bogus alias set > from a new MEM. > This looks much better to me from robustness point of view. Thanks, Honza ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 7:11 ` Jason Merrill 2001-08-15 7:13 ` Jan Hubicka @ 2001-08-15 7:36 ` Jan Hubicka 2001-08-15 8:04 ` Mark Mitchell 2 siblings, 0 replies; 24+ messages in thread From: Jan Hubicka @ 2001-08-15 7:36 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, Richard Kenner, gcc-patches > This should avoid such problems. > > 2001-08-15 Jason Merrill <jason_merrill@redhat.com> > > * explow.c (set_mem_attributes): Avoid returning a bogus alias set > from a new MEM. > Hi, I've verified that it solves my testcase. Thanks Honza ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 7:11 ` Jason Merrill 2001-08-15 7:13 ` Jan Hubicka 2001-08-15 7:36 ` Jan Hubicka @ 2001-08-15 8:04 ` Mark Mitchell 2001-08-16 5:58 ` Jan Hubicka 2 siblings, 1 reply; 24+ messages in thread From: Mark Mitchell @ 2001-08-15 8:04 UTC (permalink / raw) To: Jason Merrill, Jan Hubicka; +Cc: Richard Kenner, gcc-patches --On Wednesday, August 15, 2001 03:10:40 PM +0100 Jason Merrill <jason_merrill@redhat.com> wrote: > This should avoid such problems. > > 2001-08-15 Jason Merrill <jason_merrill@redhat.com> > > * explow.c (set_mem_attributes): Avoid returning a bogus alias set > from a new MEM. Hmm. That changes the semantics somewhat, in the abstract; i.e., it takes advantage of how get_alias_set handles decls by looking at their types. If we did something smarter (putting things in subsets based on information about the scope they were in, say), then the two calls might start meaning different things. So, I don't know whether the safety you're introducing is better, or not. Actually, how does this interact with `restrict' now? That's something where I think we may need the DECL, not the TYPE. I wish I remembered this code better; I'm just raising issues in case they apply... -- Mark Mitchell mark@codesourcery.com CodeSourcery, LLC http://www.codesourcery.com ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 8:04 ` Mark Mitchell @ 2001-08-16 5:58 ` Jan Hubicka 2001-08-16 6:09 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Jan Hubicka @ 2001-08-16 5:58 UTC (permalink / raw) To: Mark Mitchell; +Cc: Jason Merrill, Jan Hubicka, Richard Kenner, gcc-patches > > > --On Wednesday, August 15, 2001 03:10:40 PM +0100 Jason Merrill > <jason_merrill@redhat.com> wrote: > > > This should avoid such problems. > > > > 2001-08-15 Jason Merrill <jason_merrill@redhat.com> > > > > * explow.c (set_mem_attributes): Avoid returning a bogus alias set > > from a new MEM. > > Hmm. That changes the semantics somewhat, in the abstract; i.e., > it takes advantage of how get_alias_set handles decls by looking at > their types. If we did something smarter (putting things in subsets > based on information about the scope they were in, say), then the > two calls might start meaning different things. > > So, I don't know whether the safety you're introducing is better, or > not. As suggested by Richard, it probably makes sense to do this trick only as sanity checking, so following patch fixes all callers a first step. I've regtested and bootstrapped following patch together with modified set_mem_attributes patch to abort in incorrect cases. Except for simple grep it has caught problem in function.c that does replace REG by memory in place. The fix is rather ugly, but I can't come with something better. The patch above should be probably updated to abort in first case if testing succeds. OK to install? Honza Thu Aug 16 14:50:50 CEST 2001 Jan Hubicka <jh@suse.cz> * function.c (put_var_into_stack): Temporarily clear DECL_RTL. (assign_params): Avoid setting DECL_RTL to unfinished RTX. (expand_function_start): Likewise. * stmt.c (expand_decl): Likewise. * varasm.c (make_decl_rtx): Likewise. Index: function.c =================================================================== RCS file: /cvs/gcc/egcs/gcc/function.c,v retrieving revision 1.295 diff -c -3 -p -r1.295 function.c *** function.c 2001/08/13 15:52:18 1.295 --- function.c 2001/08/16 12:50:02 *************** put_var_into_stack (decl) *** 1420,1426 **** --- 1420,1433 ---- /* Change the CONCAT into a combined MEM for both parts. */ PUT_CODE (reg, MEM); + + /* set_mem_attributes uses DECL_RTL to avoid re-generating of + already computed alias sets. Here we want to re-generate. */ + if (TREE_CODE (decl) != SAVE_EXPR) + SET_DECL_RTL (decl, NULL); set_mem_attributes (reg, decl, 1); + if (TREE_CODE (decl) != SAVE_EXPR) + SET_DECL_RTL (decl, reg); /* The two parts are in memory order already. Use the lower parts address as ours. */ *************** assign_parms (fndecl) *** 4688,4697 **** appropriately. */ if (passed_pointer) { ! SET_DECL_RTL (parm, ! gen_rtx_MEM (TYPE_MODE (TREE_TYPE (passed_type)), ! parmreg)); ! set_mem_attributes (DECL_RTL (parm), parm, 1); } else { --- 4695,4704 ---- appropriately. */ if (passed_pointer) { ! rtx x = gen_rtx_MEM (TYPE_MODE (TREE_TYPE (passed_type)), ! parmreg); ! set_mem_attributes (x, parm, 1); ! SET_DECL_RTL (parm, x); } else { *************** assign_parms (fndecl) *** 5030,5040 **** if (parm == function_result_decl) { tree result = DECL_RESULT (fndecl); ! ! SET_DECL_RTL (result, ! gen_rtx_MEM (DECL_MODE (result), DECL_RTL (parm))); ! set_mem_attributes (DECL_RTL (result), result, 1); } } --- 5037,5046 ---- if (parm == function_result_decl) { tree result = DECL_RESULT (fndecl); ! rtx x = gen_rtx_MEM (DECL_MODE (result), DECL_RTL (parm)); ! set_mem_attributes (x, result, 1); ! SET_DECL_RTL (result, x); } } *************** expand_function_start (subr, parms_have_ *** 6451,6461 **** } if (value_address) { ! SET_DECL_RTL (DECL_RESULT (subr), ! gen_rtx_MEM (DECL_MODE (DECL_RESULT (subr)), ! value_address)); ! set_mem_attributes (DECL_RTL (DECL_RESULT (subr)), ! DECL_RESULT (subr), 1); } } else if (DECL_MODE (DECL_RESULT (subr)) == VOIDmode) --- 6457,6465 ---- } if (value_address) { ! rtx x = gen_rtx_MEM (DECL_MODE (DECL_RESULT (subr)), value_address); ! set_mem_attributes (x, DECL_RESULT (subr), 1); ! SET_DECL_RTL (DECL_RESULT (subr), x); } } else if (DECL_MODE (DECL_RESULT (subr)) == VOIDmode) Index: stmt.c =================================================================== RCS file: /cvs/gcc/egcs/gcc/stmt.c,v retrieving revision 1.208 diff -c -3 -p -r1.208 stmt.c *** stmt.c 2001/08/13 15:52:22 1.208 --- stmt.c 2001/08/16 12:50:07 *************** expand_decl (decl) *** 3810,3824 **** else if (DECL_SIZE (decl) == 0) /* Variable with incomplete type. */ { if (DECL_INITIAL (decl) == 0) /* Error message was already done; now avoid a crash. */ ! SET_DECL_RTL (decl, gen_rtx_MEM (BLKmode, const0_rtx)); else /* An initializer is going to decide the size of this array. Until we know the size, represent its address with a reg. */ ! SET_DECL_RTL (decl, gen_rtx_MEM (BLKmode, gen_reg_rtx (Pmode))); ! set_mem_attributes (DECL_RTL (decl), decl, 1); } else if (DECL_MODE (decl) != BLKmode /* If -ffloat-store, don't put explicit float vars --- 3810,3826 ---- else if (DECL_SIZE (decl) == 0) /* Variable with incomplete type. */ { + rtx x; if (DECL_INITIAL (decl) == 0) /* Error message was already done; now avoid a crash. */ ! x = gen_rtx_MEM (BLKmode, const0_rtx); else /* An initializer is going to decide the size of this array. Until we know the size, represent its address with a reg. */ ! x = gen_rtx_MEM (BLKmode, gen_reg_rtx (Pmode)); ! set_mem_attributes (x, decl, 1); ! SET_DECL_RTL (decl, x); } else if (DECL_MODE (decl) != BLKmode /* If -ffloat-store, don't put explicit float vars *************** expand_decl (decl) *** 3888,3894 **** else /* Dynamic-size object: must push space on the stack. */ { ! rtx address, size; /* Record the stack pointer on entry to block, if have not already done so. */ --- 3890,3896 ---- else /* Dynamic-size object: must push space on the stack. */ { ! rtx address, size, x; /* Record the stack pointer on entry to block, if have not already done so. */ *************** expand_decl (decl) *** 3913,3921 **** TYPE_ALIGN (TREE_TYPE (decl))); /* Reference the variable indirect through that rtx. */ ! SET_DECL_RTL (decl, gen_rtx_MEM (DECL_MODE (decl), address)); - set_mem_attributes (DECL_RTL (decl), decl, 1); /* Indicate the alignment we actually gave this variable. */ #ifdef STACK_BOUNDARY --- 3915,3924 ---- TYPE_ALIGN (TREE_TYPE (decl))); /* Reference the variable indirect through that rtx. */ ! x = gen_rtx_MEM (DECL_MODE (decl), address); ! set_mem_attributes (x, decl, 1); ! SET_DECL_RTL (decl, x); /* Indicate the alignment we actually gave this variable. */ #ifdef STACK_BOUNDARY Index: varasm.c =================================================================== RCS file: /cvs/gcc/egcs/gcc/varasm.c,v retrieving revision 1.193 diff -c -3 -p -r1.193 varasm.c *** varasm.c 2001/08/13 15:14:52 1.193 --- varasm.c 2001/08/16 12:50:09 *************** make_decl_rtl (decl, asmspec) *** 591,596 **** --- 591,597 ---- const char *name = 0; const char *new_name = 0; int reg_number; + rtx x; /* Check that we are not being given an automatic variable. */ /* A weak alias has TREE_PUBLIC set but not the other bits. */ *************** make_decl_rtl (decl, asmspec) *** 758,768 **** && (TREE_PUBLIC (decl) || TREE_STATIC (decl))))) TREE_SIDE_EFFECTS (decl) = 1; ! SET_DECL_RTL (decl, gen_rtx_MEM (DECL_MODE (decl), ! gen_rtx_SYMBOL_REF (Pmode, name))); ! SYMBOL_REF_WEAK (XEXP (DECL_RTL (decl), 0)) = DECL_WEAK (decl); if (TREE_CODE (decl) != FUNCTION_DECL) ! set_mem_attributes (DECL_RTL (decl), decl, 1); /* Optionally set flags or add text to the name to record information such as that it is a function name. --- 759,769 ---- && (TREE_PUBLIC (decl) || TREE_STATIC (decl))))) TREE_SIDE_EFFECTS (decl) = 1; ! x = gen_rtx_MEM (DECL_MODE (decl), gen_rtx_SYMBOL_REF (Pmode, name)); ! SYMBOL_REF_WEAK (XEXP (x, 0)) = DECL_WEAK (decl); if (TREE_CODE (decl) != FUNCTION_DECL) ! set_mem_attributes (x, decl, 1); ! SET_DECL_RTL (decl, x); /* Optionally set flags or add text to the name to record information such as that it is a function name. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-16 5:58 ` Jan Hubicka @ 2001-08-16 6:09 ` Jason Merrill 2001-08-16 7:40 ` Jan Hubicka 0 siblings, 1 reply; 24+ messages in thread From: Jason Merrill @ 2001-08-16 6:09 UTC (permalink / raw) To: Jan Hubicka; +Cc: Mark Mitchell, Richard Kenner, gcc-patches >>>>> "Jan" == Jan Hubicka <jh@suse.cz> writes: > + /* set_mem_attributes uses DECL_RTL to avoid re-generating of > + already computed alias sets. Here we want to re-generate. */ > + if (TREE_CODE (decl) != SAVE_EXPR) > + SET_DECL_RTL (decl, NULL); > set_mem_attributes (reg, decl, 1); > + if (TREE_CODE (decl) != SAVE_EXPR) > + SET_DECL_RTL (decl, reg); This should check DECL_P. Otherwise, looks good. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-16 6:09 ` Jason Merrill @ 2001-08-16 7:40 ` Jan Hubicka 2001-08-16 7:46 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Jan Hubicka @ 2001-08-16 7:40 UTC (permalink / raw) To: Jason Merrill; +Cc: Jan Hubicka, Mark Mitchell, Richard Kenner, gcc-patches > >>>>> "Jan" == Jan Hubicka <jh@suse.cz> writes: > > > + /* set_mem_attributes uses DECL_RTL to avoid re-generating of > > + already computed alias sets. Here we want to re-generate. */ > > + if (TREE_CODE (decl) != SAVE_EXPR) > > + SET_DECL_RTL (decl, NULL); > > set_mem_attributes (reg, decl, 1); > > + if (TREE_CODE (decl) != SAVE_EXPR) > > + SET_DECL_RTL (decl, reg); > > This should check DECL_P. Otherwise, looks good. May I interpret this as approval with this change? > > Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-16 7:40 ` Jan Hubicka @ 2001-08-16 7:46 ` Jason Merrill 0 siblings, 0 replies; 24+ messages in thread From: Jason Merrill @ 2001-08-16 7:46 UTC (permalink / raw) To: Jan Hubicka; +Cc: Mark Mitchell, Richard Kenner, gcc-patches >>>>> "Jan" == Jan Hubicka <jh@suse.cz> writes: >> >>>>> "Jan" == Jan Hubicka <jh@suse.cz> writes: >> >> > + /* set_mem_attributes uses DECL_RTL to avoid re-generating of >> > + already computed alias sets. Here we want to re-generate. */ >> > + if (TREE_CODE (decl) != SAVE_EXPR) >> > + SET_DECL_RTL (decl, NULL); >> > set_mem_attributes (reg, decl, 1); >> > + if (TREE_CODE (decl) != SAVE_EXPR) >> > + SET_DECL_RTL (decl, reg); >> >> This should check DECL_P. Otherwise, looks good. > May I interpret this as approval with this change? Yes. Please apply all the changes you have; I haven't checked in any of the patches I've sent out. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression @ 2001-08-15 11:28 Richard Kenner 2001-08-16 6:09 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Richard Kenner @ 2001-08-15 11:28 UTC (permalink / raw) To: jason_merrill; +Cc: gcc-patches ! if (DECL_P (t) && ref == DECL_RTL_IF_SET (t)) ! /* Avoid returning a bogus alias set from a new MEM. */ ! set_mem_alias_set (ref, get_alias_set (type)); ! else ! set_mem_alias_set (ref, get_alias_set (t)); I suppose, but I think this is a real kludge. Instead, I'd abort in the first case and fix all the callers. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 11:28 Richard Kenner @ 2001-08-16 6:09 ` Jason Merrill 2001-08-22 7:53 ` Jason Merrill 0 siblings, 1 reply; 24+ messages in thread From: Jason Merrill @ 2001-08-16 6:09 UTC (permalink / raw) To: Richard Kenner; +Cc: gcc-patches >>>>> "Richard" == Richard Kenner <kenner@vlsi1.ultra.nyu.edu> writes: > ! if (DECL_P (t) && ref == DECL_RTL_IF_SET (t)) > ! /* Avoid returning a bogus alias set from a new MEM. */ > ! set_mem_alias_set (ref, get_alias_set (type)); > ! else > ! set_mem_alias_set (ref, get_alias_set (t)); > I suppose, but I think this is a real kludge. > Instead, I'd abort in the first case and fix all the callers. Makes sense. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-16 6:09 ` Jason Merrill @ 2001-08-22 7:53 ` Jason Merrill 0 siblings, 0 replies; 24+ messages in thread From: Jason Merrill @ 2001-08-22 7:53 UTC (permalink / raw) To: Richard Kenner; +Cc: gcc-patches >>>>> "Jason" == Jason Merrill <jason_merrill@redhat.com> writes: >> Instead, I'd abort in the first case and fix all the callers. > Makes sense. Applied: 2001-08-22 Jason Merrill <jason_merrill@redhat.com> * explow.c (set_mem_attributes): Avoid returning a bogus alias set from a new MEM. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression @ 2001-08-15 7:08 Richard Kenner 2001-08-15 7:10 ` Jan Hubicka 0 siblings, 1 reply; 24+ messages in thread From: Richard Kenner @ 2001-08-15 7:08 UTC (permalink / raw) To: jh; +Cc: gcc-patches If I understand the purpose of original patch properly, it was hooting for case variable gets unique class, but this class is recomputed several times so it get several unique classes. For class 0 this does not matter. No, that's backwards. This is a case where the RTL was class 0 and so you want to make sure the new class is 0. Your change will cause it to use the alias set of the type, which might be nonzero and hence is wrong. ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 7:08 Richard Kenner @ 2001-08-15 7:10 ` Jan Hubicka 0 siblings, 0 replies; 24+ messages in thread From: Jan Hubicka @ 2001-08-15 7:10 UTC (permalink / raw) To: Richard Kenner; +Cc: jh, gcc-patches > If I understand the purpose of original patch properly, it was hooting > for case variable gets unique class, but this class is recomputed > several times so it get several unique classes. For class 0 this does > not matter. > > No, that's backwards. This is a case where the RTL was class 0 and so you > want to make sure the new class is 0. Your change will cause it to use > the alias set of the type, which might be nonzero and hence is wrong. Uhh, I see, then we probably need to go with Jason's patch, as I don't see any way how to check case someone call set_memory_attributes after doing DECL_RTL... Honza ^ permalink raw reply [flat|nested] 24+ messages in thread
[parent not found: <20010815123730.P19872@atrey.karlin.mff.cuni.cz>]
[parent not found: <m366bpsi91.fsf@prospero.cambridge.redhat.com>]
* Re: spec2000 regression [not found] ` <m366bpsi91.fsf@prospero.cambridge.redhat.com> @ 2001-08-15 5:41 ` Jan Hubicka 2001-08-15 6:18 ` Jason Merrill ` (2 more replies) 0 siblings, 3 replies; 24+ messages in thread From: Jan Hubicka @ 2001-08-15 5:41 UTC (permalink / raw) To: Jason Merrill, gcc-patches, rth, aj; +Cc: Jan Hubicka, gcc > >>>>> "Jan" == Jan Hubicka <jh@suse.cz> writes: > > > So remaining patch I can't analyze completely is yours to alias.c > > I've failed to find the corresponding email at gcc-patches mailing list > > (even when I believe I've went across it in the pass) > > The subject line is "PATCH to get_alias_set". There were two messages. > > > so please can you double-check the patch and try to explain me what > > exactly it is shooting for? > > It's trying to avoid recalculating the alias set for a variable. I suppose > it could cause a performance regression if the initial alias set were > wrong, but I would expect that to cause other problems as well. Hi, the problem is that actually code first sets the DECL_RTL and then calls set_mem_attributes, that calls get_alias_set, but it thinks that the alias set is already computed, but it isn't. I am now testing following patch. OK to install if it succeeds? It fixes my testcase: long *a; short *b; main() { *a = 1; *b = 2; return *a; } Now gets again: pushl %ebp movl %esp, %ebp pushl %eax pushl %eax movl a, %eax andl $-16, %esp movl $1, (%eax) movl b, %eax movw $2, (%eax) movl %ebp, %esp movl $1, %eax popl %ebp ret Honza Wed Aug 15 14:38:26 CEST 2001 Jan Hubicka <jh@suse.cz> * alias.c (get_alias_set): Check that the alias set is already computed. Index: alias.c =================================================================== RCS file: /cvs/gcc/egcs/gcc/alias.c,v retrieving revision 1.137 diff -c -3 -p -r1.137 alias.c *** alias.c 2001/08/08 16:56:50 1.137 --- alias.c 2001/08/15 12:38:13 *************** get_alias_set (t) *** 530,536 **** return it. This is necessary for C++ anonymous unions, whose component variables don't look like union members (boo!). */ if (TREE_CODE (t) == VAR_DECL ! && DECL_RTL_SET_P (t) && GET_CODE (DECL_RTL (t)) == MEM) return MEM_ALIAS_SET (DECL_RTL (t)); /* Give the language another chance to do something special. */ --- 530,537 ---- return it. This is necessary for C++ anonymous unions, whose component variables don't look like union members (boo!). */ if (TREE_CODE (t) == VAR_DECL ! && DECL_RTL_SET_P (t) && GET_CODE (DECL_RTL (t)) == MEM ! && MEM_ALIAS_SET (DECL_RTL (t))) return MEM_ALIAS_SET (DECL_RTL (t)); /* Give the language another chance to do something special. */ ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 5:41 ` Jan Hubicka @ 2001-08-15 6:18 ` Jason Merrill 2001-08-15 6:40 ` Joseph S. Myers 2001-08-15 7:56 ` Mark Mitchell 2 siblings, 0 replies; 24+ messages in thread From: Jason Merrill @ 2001-08-15 6:18 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches, rth, aj, gcc >>>>> "Jan" == Jan Hubicka <jh@suse.cz> writes: > Hi, the problem is that actually code first sets the DECL_RTL and then > calls set_mem_attributes, that calls get_alias_set, but it thinks that > the alias set is already computed, but it isn't. Makes sense. > I am now testing following patch. OK to install if it succeeds? No, we might want alias set 0. Rather, the code shouldn't set DECL_RTL until it's done building up the RTL. Jason ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 5:41 ` Jan Hubicka 2001-08-15 6:18 ` Jason Merrill @ 2001-08-15 6:40 ` Joseph S. Myers 2001-08-15 7:36 ` Jan Hubicka 2001-08-15 7:56 ` Mark Mitchell 2 siblings, 1 reply; 24+ messages in thread From: Joseph S. Myers @ 2001-08-15 6:40 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Wed, 15 Aug 2001, Jan Hubicka wrote: > I am now testing following patch. OK to install if it succeeds? You should add a testcase to the testsuite for this bug. You can do this by calling a non-existent function such that the call will be optimised away if the optimisation succeeds. (Look e.g. at gcc.c-torture/execute/builtin-abs-1.c - in this case you'll want to use 'set additional_flags "-fstrict-aliasing"' in the .x file so that the optimisation works at -O1 (which doesn't include -fstrict-aliasing) and #ifndef __OPTIMIZE__ is the only test needed for the dummy version of the function that aborts.) -- Joseph S. Myers jsm28@cam.ac.uk ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 6:40 ` Joseph S. Myers @ 2001-08-15 7:36 ` Jan Hubicka 2001-08-15 7:44 ` Joseph S. Myers 0 siblings, 1 reply; 24+ messages in thread From: Jan Hubicka @ 2001-08-15 7:36 UTC (permalink / raw) To: Joseph S. Myers; +Cc: Jan Hubicka, gcc-patches > On Wed, 15 Aug 2001, Jan Hubicka wrote: > > > I am now testing following patch. OK to install if it succeeds? > > You should add a testcase to the testsuite for this bug. You can do this > by calling a non-existent function such that the call will be optimised > away if the optimisation succeeds. (Look e.g. at > gcc.c-torture/execute/builtin-abs-1.c - in this case you'll want to use > 'set additional_flags "-fstrict-aliasing"' in the .x file so that the > optimisation works at -O1 (which doesn't include -fstrict-aliasing) and > #ifndef __OPTIMIZE__ is the only test needed for the dummy version of the > function that aborts.) OK, Would be following testsuite OK? long long a1, *a=&a1; short b1, *b=&b1; main() { #ifdef __OPTIMIZE__ *a = 1; *b = 2; if (*a != 1) undefined (); #endif return 0; } ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 7:36 ` Jan Hubicka @ 2001-08-15 7:44 ` Joseph S. Myers 2001-08-15 11:04 ` Andreas Jaeger 0 siblings, 1 reply; 24+ messages in thread From: Joseph S. Myers @ 2001-08-15 7:44 UTC (permalink / raw) To: Jan Hubicka; +Cc: gcc-patches On Wed, 15 Aug 2001, Jan Hubicka wrote: > Would be following testsuite OK? > > long long a1, *a=&a1; > short b1, *b=&b1; > main() > { > #ifdef __OPTIMIZE__ > *a = 1; > *b = 2; > if (*a != 1) > undefined (); > #endif > return 0; > } Assuming that it does indeed pass with working compilers but fails with mainline before the problem is fixed, that testcase should be OK. -- Joseph S. Myers jsm28@cam.ac.uk ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 7:44 ` Joseph S. Myers @ 2001-08-15 11:04 ` Andreas Jaeger 0 siblings, 0 replies; 24+ messages in thread From: Andreas Jaeger @ 2001-08-15 11:04 UTC (permalink / raw) To: Joseph S. Myers; +Cc: Jan Hubicka, gcc-patches "Joseph S. Myers" <jsm28@cam.ac.uk> writes: > On Wed, 15 Aug 2001, Jan Hubicka wrote: > >> Would be following testsuite OK? >> >> long long a1, *a=&a1; >> short b1, *b=&b1; >> main() >> { >> #ifdef __OPTIMIZE__ >> *a = 1; >> *b = 2; >> if (*a != 1) >> undefined (); >> #endif >> return 0; >> } > > Assuming that it does indeed pass with working compilers but fails with > mainline before the problem is fixed, that testcase should be OK. It works with 3.0 and fails with 3.1 CVS from yesterday: gromit:/tmp:[127]$ /opt/gcc-3.1-devel/bin/gcc -Wall t.c -O2 t.c:4: warning: return type defaults to `int' t.c: In function `main': t.c:9: warning: implicit declaration of function `undefined' /tmp/cc9lSIBE.o: In function `main': /tmp/cc9lSIBE.o(.text+0x41): undefined reference to `undefined' collect2: ld returned 1 exit status gromit:/tmp:[1]$ /opt/gcc-3.0/bin/gcc -Wall t.c -O2 t.c:4: warning: return type defaults to `int' t.c: In function `main': t.c:9: warning: implicit declaration of function `undefined' I didn't check any of the patches floating around. But shouldn't the testcase get fixed so that it issues no warnings? Andreas -- Andreas Jaeger SuSE Labs aj@suse.de private aj@arthur.inka.de http://www.suse.de/~aj ^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: spec2000 regression 2001-08-15 5:41 ` Jan Hubicka 2001-08-15 6:18 ` Jason Merrill 2001-08-15 6:40 ` Joseph S. Myers @ 2001-08-15 7:56 ` Mark Mitchell 2 siblings, 0 replies; 24+ messages in thread From: Mark Mitchell @ 2001-08-15 7:56 UTC (permalink / raw) To: Jan Hubicka, Jason Merrill, gcc-patches, rth, aj; +Cc: gcc --On Wednesday, August 15, 2001 02:41:27 PM +0200 Jan Hubicka <jh@suse.cz> wrote: > the problem is that actually code first sets the DECL_RTL and then calls > set_mem_attributes, that calls get_alias_set, but it thinks that the > alias set is already computed, but it isn't. > That patch doesn't look right to me. You should figure out why the alias set is already initialized, and clear it if necessary before you get here. -- Mark Mitchell mark@codesourcery.com CodeSourcery, LLC http://www.codesourcery.com ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2001-08-22 7:53 UTC | newest] Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2001-08-15 5:57 spec2000 regression Richard Kenner 2001-08-15 6:03 ` Jan Hubicka 2001-08-15 6:36 ` Jason Merrill 2001-08-15 6:52 ` Jan Hubicka 2001-08-15 7:11 ` Jason Merrill 2001-08-15 7:13 ` Jan Hubicka 2001-08-15 7:36 ` Jan Hubicka 2001-08-15 8:04 ` Mark Mitchell 2001-08-16 5:58 ` Jan Hubicka 2001-08-16 6:09 ` Jason Merrill 2001-08-16 7:40 ` Jan Hubicka 2001-08-16 7:46 ` Jason Merrill -- strict thread matches above, loose matches on Subject: below -- 2001-08-15 11:28 Richard Kenner 2001-08-16 6:09 ` Jason Merrill 2001-08-22 7:53 ` Jason Merrill 2001-08-15 7:08 Richard Kenner 2001-08-15 7:10 ` Jan Hubicka [not found] <20010815123730.P19872@atrey.karlin.mff.cuni.cz> [not found] ` <m366bpsi91.fsf@prospero.cambridge.redhat.com> 2001-08-15 5:41 ` Jan Hubicka 2001-08-15 6:18 ` Jason Merrill 2001-08-15 6:40 ` Joseph S. Myers 2001-08-15 7:36 ` Jan Hubicka 2001-08-15 7:44 ` Joseph S. Myers 2001-08-15 11:04 ` Andreas Jaeger 2001-08-15 7:56 ` Mark Mitchell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).