| Message ID | 20130304211744.GX12913@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 4, 2013 at 10:17 PM, Jakub Jelinek wrote: > Hi! > > Something that again hits lots of testcases during valgrind checking > bootstrap. init_alias_analysis apparently does > vec_safe_grow_cleared (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER); > reg_known_equiv_p = sbitmap_alloc (maxreg - FIRST_PSEUDO_REGISTER); > but doesn't bitmap_clear (reg_known_equiv_p), perhaps as an optimization? No, an incorrect replacement with sbitmap. We used to have: reg_known_equiv_p = XCNEWVEC (bool, reg_known_value_size); I'm probably to blame for this one, actually :-) Ciao! Steven
On Mon, Mar 4, 2013 at 10:17 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > Something that again hits lots of testcases during valgrind checking > bootstrap. init_alias_analysis apparently does > vec_safe_grow_cleared (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER); > reg_known_equiv_p = sbitmap_alloc (maxreg - FIRST_PSEUDO_REGISTER); > but doesn't bitmap_clear (reg_known_equiv_p), perhaps as an optimization? > If set_reg_known_value is called (and not to the reg itself), > set_reg_known_equiv_p is called too though. > Right now get_reg_known_equiv_p is only called in one place, and we are only > interested in MEM_P known values there, so the following works fine. > Though perhaps if in the future we use the reg_known_equiv_p bitmap more, > we should bitmap_clear (reg_known_equiv_p) it instead. > Bootstrapped/regtested on x86_64-linux and i686-linux. > > Ok for trunk (or do you prefer to slow down init_alias_analysis and just > clear the bitmap)? Looks ok, also clear the sbitmap as of stevens comment. Thanks, Richard. > 2013-03-04 Jakub Jelinek <jakub@redhat.com> > > * sched-deps.c (sched_analyze_reg): Only call get_reg_known_equiv_p > if get_reg_known_value returned non-NULL. > > --- gcc/sched-deps.c.jj 2013-03-04 12:21:09.000000000 +0100 > +++ gcc/sched-deps.c 2013-03-04 17:29:03.478944157 +0100 > @@ -2351,10 +2351,10 @@ sched_analyze_reg (struct deps_desc *dep > /* Pseudos that are REG_EQUIV to something may be replaced > by that during reloading. We need only add dependencies for > the address in the REG_EQUIV note. */ > - if (!reload_completed && get_reg_known_equiv_p (regno)) > + if (!reload_completed) > { > rtx t = get_reg_known_value (regno); > - if (MEM_P (t)) > + if (t && MEM_P (t) && get_reg_known_equiv_p (regno)) > sched_analyze_2 (deps, XEXP (t, 0), insn); > } > > > Jakub
On 13-03-04 4:17 PM, Jakub Jelinek wrote: > Hi! > > Something that again hits lots of testcases during valgrind checking > bootstrap. init_alias_analysis apparently does > vec_safe_grow_cleared (reg_known_value, maxreg - FIRST_PSEUDO_REGISTER); > reg_known_equiv_p = sbitmap_alloc (maxreg - FIRST_PSEUDO_REGISTER); > but doesn't bitmap_clear (reg_known_equiv_p), perhaps as an optimization? Sorry, I don't know current state of alias.c well to say something definite about this. But I believe it should be cleared. > If set_reg_known_value is called (and not to the reg itself), > set_reg_known_equiv_p is called too though. > Right now get_reg_known_equiv_p is only called in one place, and we are only > interested in MEM_P known values there, so the following works fine. > Though perhaps if in the future we use the reg_known_equiv_p bitmap more, > we should bitmap_clear (reg_known_equiv_p) it instead. > Bootstrapped/regtested on x86_64-linux and i686-linux. > > Ok for trunk (or do you prefer to slow down init_alias_analysis and just > clear the bitmap)? I don't see any harm from your patch but I guess it should be fixed by clearing reg_know_equiv_p. I think you need Steven's opinion on this as he is an author of the code. > > 2013-03-04 Jakub Jelinek <jakub@redhat.com> > > * sched-deps.c (sched_analyze_reg): Only call get_reg_known_equiv_p > if get_reg_known_value returned non-NULL. > > --- gcc/sched-deps.c.jj 2013-03-04 12:21:09.000000000 +0100 > +++ gcc/sched-deps.c 2013-03-04 17:29:03.478944157 +0100 > @@ -2351,10 +2351,10 @@ sched_analyze_reg (struct deps_desc *dep > /* Pseudos that are REG_EQUIV to something may be replaced > by that during reloading. We need only add dependencies for > the address in the REG_EQUIV note. */ > - if (!reload_completed && get_reg_known_equiv_p (regno)) > + if (!reload_completed) > { > rtx t = get_reg_known_value (regno); > - if (MEM_P (t)) > + if (t && MEM_P (t) && get_reg_known_equiv_p (regno)) > sched_analyze_2 (deps, XEXP (t, 0), insn); > } > > >
On Tue, Mar 05, 2013 at 11:58:09PM -0500, Vladimir Makarov wrote: > I don't see any harm from your patch but I guess it should be fixed > by clearing reg_know_equiv_p. I think you need Steven's opinion on > this as he is an author of the code. Yeah, I've already committed the clearing of the sbitmap in alias.c instead of this sched-deps.c patch, which doesn't make sense after the alias.c change. Jakub
--- gcc/sched-deps.c.jj 2013-03-04 12:21:09.000000000 +0100 +++ gcc/sched-deps.c 2013-03-04 17:29:03.478944157 +0100 @@ -2351,10 +2351,10 @@ sched_analyze_reg (struct deps_desc *dep /* Pseudos that are REG_EQUIV to something may be replaced by that during reloading. We need only add dependencies for the address in the REG_EQUIV note. */ - if (!reload_completed && get_reg_known_equiv_p (regno)) + if (!reload_completed) { rtx t = get_reg_known_value (regno); - if (MEM_P (t)) + if (t && MEM_P (t) && get_reg_known_equiv_p (regno)) sched_analyze_2 (deps, XEXP (t, 0), insn); }