| Message ID | 20101110231619.GY29412@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Nov 11, 2010 at 12:16 AM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > On IA-64 the attached testcase ICEs, because a MEM isn't properly > invalidated in cselib which causes ICE when handling a COND_EXEC > storing to that MEM. > > The problem is in the {sp,hfp}+off -> {cfa_base_rtx}+off2 replacements > done by var-tracking, although the whole IL is transformed that way, > on the side stuff computed by init_alias_analysis is not, and > as alias analysis normally expects that argp based memory can't > alias with sp based memory or hfp based memory (which is > certainly true before reload and after reload argp shouldn't be normally > used in the IL), find_base_term can be different between the > hard reg containing some sp (or hfp) based address and VALUE > corresponding to it. > > Fixed by telling (temporarily) alias.c that argp (resp. fp) > can during var-tracking alias with sp (resp. hfp) based addresses. > > Bootstrapped/regtested on x86_64-linux and i686-linux and tested on > the testcase with ia64-linux cross. Ok for trunk? > > 2010-11-09 Jakub Jelinek <jakub@redhat.com> > > PR debug/46387 > * rtl.h (equate_reg_base_value): New prototype. > * alias.c (equate_reg_base_value): New function. > * var-tracking.c (vt_init_cfa_base): Use it. > (vt_finalize): Likewise. Clear cfa_base_rtx here... > (vt_initialize): ... instead of here. > > * gcc.dg/pr46387.c: New test. > > --- gcc/rtl.h.jj 2010-11-03 16:58:17.000000000 +0100 > +++ gcc/rtl.h 2010-11-09 19:49:35.000000000 +0100 > @@ -2503,6 +2503,7 @@ extern int may_alias_p (const_rtx, const > extern void init_alias_target (void); > extern void init_alias_analysis (void); > extern void end_alias_analysis (void); > +extern void equate_reg_base_value (const_rtx, const_rtx); > extern bool memory_modified_in_insn_p (const_rtx, const_rtx); > extern rtx find_base_term (rtx); > extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int); > --- gcc/alias.c.jj 2010-11-01 09:07:23.000000000 +0100 > +++ gcc/alias.c 2010-11-09 19:49:24.000000000 +0100 > @@ -2906,6 +2906,26 @@ init_alias_analysis (void) > timevar_pop (TV_ALIAS_ANALYSIS); > } > > +/* Equate REG_BASE_VALUE (reg1) to REG_BASE_VALUE (reg2), > + or reset it back to its original value if reg2 == reg1. */ > + > +void > +equate_reg_base_value (const_rtx reg1, const_rtx reg2) > +{ > + gcc_assert (REGNO (reg1) < FIRST_PSEUDO_REGISTER > + && REGNO (reg2) < FIRST_PSEUDO_REGISTER); > + if (reg1 == reg2) > + VEC_replace (rtx, reg_base_value, REGNO (reg1), > + static_reg_base_value[REGNO (reg1)]); That's not ok - static_reg_base_value is only used as initial value for the iteration that will eventually end up invalidating it, so you can't simply re-set it to that value. Why reset it at all? I think we do init_alias_analysis () everywhere we need it. Richard. > + else > + { > + gcc_assert (REG_BASE_VALUE (reg1) > + == static_reg_base_value[REGNO (reg1)]); > + VEC_replace (rtx, reg_base_value, REGNO (reg1), > + REG_BASE_VALUE (reg2)); > + } > +} > + > void > end_alias_analysis (void) > { > --- gcc/var-tracking.c.jj 2010-11-09 13:58:30.000000000 +0100 > +++ gcc/var-tracking.c 2010-11-09 19:53:52.000000000 +0100 > @@ -8229,6 +8229,11 @@ vt_init_cfa_base (void) > if (!MAY_HAVE_DEBUG_INSNS) > return; > > + /* Tell alias analysis that cfa_base_rtx should temporarily share > + find_base_term value with stack pointer or hard frame pointer. */ > + equate_reg_base_value (cfa_base_rtx, > + frame_pointer_needed > + ? hard_frame_pointer_rtx : stack_pointer_rtx); > val = cselib_lookup_from_insn (cfa_base_rtx, GET_MODE (cfa_base_rtx), 1, > get_insns ()); > preserve_value (val); > @@ -8460,7 +8465,6 @@ vt_initialize (void) > hard_frame_pointer_adjustment = -1; > VTI (ENTRY_BLOCK_PTR)->flooded = true; > vt_add_function_parameters (); > - cfa_base_rtx = NULL_RTX; > return true; > } > > @@ -8503,6 +8507,10 @@ vt_finalize (void) > { > basic_block bb; > > + if (cfa_base_rtx && MAY_HAVE_DEBUG_INSNS) > + equate_reg_base_value (cfa_base_rtx, cfa_base_rtx); > + cfa_base_rtx = NULL_RTX; > + > FOR_EACH_BB (bb) > { > VEC_free (micro_operation, heap, VTI (bb)->mos); > --- gcc/testsuite/gcc.dg/pr46387.c.jj 2010-11-09 20:35:53.000000000 +0100 > +++ gcc/testsuite/gcc.dg/pr46387.c 2010-11-09 20:35:44.000000000 +0100 > @@ -0,0 +1,32 @@ > +/* PR debug/46387 */ > +/* { dg-do compile } */ > +/* { dg-options "-g -O2" } */ > + > +struct S { double x; double y; short z; }; > +int a = 0, b = 0, c; > +void bar (int, int, int); > +void baz (int *, int *, int *); > + > +void > +foo (struct S *v) > +{ > + int x, y, z; > + if (!a && b != 0) > + return; > + if (v->z) > + baz (&x, &y, &z); > + else > + { > + x = v->x; > + y = v->y; > + } > + x = x / (5 + 1); > + y = y / (5 + 1); > + if (x < 0) > + x = 0; > + if (x > c - 1) > + x = c - 1; > + if (b == 0) > + bar (x, y, 1); > + return; > +} > > Jakub >
On Thu, Nov 11, 2010 at 11:04:49AM +0100, Richard Guenther wrote: > On Thu, Nov 11, 2010 at 12:16 AM, Jakub Jelinek <jakub@redhat.com> wrote: > > +/* Equate REG_BASE_VALUE (reg1) to REG_BASE_VALUE (reg2), > > + or reset it back to its original value if reg2 == reg1. */ > > + > > +void > > +equate_reg_base_value (const_rtx reg1, const_rtx reg2) > > +{ > > + gcc_assert (REGNO (reg1) < FIRST_PSEUDO_REGISTER > > + && REGNO (reg2) < FIRST_PSEUDO_REGISTER); > > + if (reg1 == reg2) > > + VEC_replace (rtx, reg_base_value, REGNO (reg1), > > + static_reg_base_value[REGNO (reg1)]); > > That's not ok - static_reg_base_value is only used as initial > value for the iteration that will eventually end up invalidating > it, so you can't simply re-set it to that value. The function asserts it is equal when changing to different value, so copying static_reg_base_value is known to be the restore for it. var-tracking only does this with a reg (argp resp. framep) if it is known to be eliminated already, so it really can't have different value. > Why reset it at all? I think we do init_alias_analysis () everywhere > we need it. E.g. var-tracking.c itself doesn't call init_alias_analysis, and it wouldn't surprise me if final in a target or two didn't query alias analysis. As argp/framep should be eliminated from the IL after RA, in theory keeping it that way shouldn't cause any differences, just wanted to play safe. Jakub
On Thu, Nov 11, 2010 at 11:18 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Nov 11, 2010 at 11:04:49AM +0100, Richard Guenther wrote: >> On Thu, Nov 11, 2010 at 12:16 AM, Jakub Jelinek <jakub@redhat.com> wrote: > >> > +/* Equate REG_BASE_VALUE (reg1) to REG_BASE_VALUE (reg2), >> > + or reset it back to its original value if reg2 == reg1. */ >> > + >> > +void >> > +equate_reg_base_value (const_rtx reg1, const_rtx reg2) >> > +{ >> > + gcc_assert (REGNO (reg1) < FIRST_PSEUDO_REGISTER >> > + && REGNO (reg2) < FIRST_PSEUDO_REGISTER); >> > + if (reg1 == reg2) >> > + VEC_replace (rtx, reg_base_value, REGNO (reg1), >> > + static_reg_base_value[REGNO (reg1)]); >> >> That's not ok - static_reg_base_value is only used as initial >> value for the iteration that will eventually end up invalidating >> it, so you can't simply re-set it to that value. > > The function asserts it is equal when changing to different value, > so copying static_reg_base_value is known to be the restore for it. > var-tracking only does this with a reg (argp resp. framep) if it is known to > be eliminated already, so it really can't have different value. Ok, so this function can't be used to fix an equivalent problem in selective scheduling (they probably also need non-hardregs). >> Why reset it at all? I think we do init_alias_analysis () everywhere >> we need it. > > E.g. var-tracking.c itself doesn't call init_alias_analysis, and it wouldn't > surprise me if final in a target or two didn't query alias analysis. > As argp/framep should be eliminated from the IL after RA, in theory keeping > it that way shouldn't cause any differences, just wanted to play safe. I see. I'm not sure I like the special just-for-vartracking function in alias.c too much (in fact I hoped we can share it for selsched). Hmm. Richard. > Jakub >
--- gcc/rtl.h.jj 2010-11-03 16:58:17.000000000 +0100 +++ gcc/rtl.h 2010-11-09 19:49:35.000000000 +0100 @@ -2503,6 +2503,7 @@ extern int may_alias_p (const_rtx, const extern void init_alias_target (void); extern void init_alias_analysis (void); extern void end_alias_analysis (void); +extern void equate_reg_base_value (const_rtx, const_rtx); extern bool memory_modified_in_insn_p (const_rtx, const_rtx); extern rtx find_base_term (rtx); extern rtx gen_hard_reg_clobber (enum machine_mode, unsigned int); --- gcc/alias.c.jj 2010-11-01 09:07:23.000000000 +0100 +++ gcc/alias.c 2010-11-09 19:49:24.000000000 +0100 @@ -2906,6 +2906,26 @@ init_alias_analysis (void) timevar_pop (TV_ALIAS_ANALYSIS); } +/* Equate REG_BASE_VALUE (reg1) to REG_BASE_VALUE (reg2), + or reset it back to its original value if reg2 == reg1. */ + +void +equate_reg_base_value (const_rtx reg1, const_rtx reg2) +{ + gcc_assert (REGNO (reg1) < FIRST_PSEUDO_REGISTER + && REGNO (reg2) < FIRST_PSEUDO_REGISTER); + if (reg1 == reg2) + VEC_replace (rtx, reg_base_value, REGNO (reg1), + static_reg_base_value[REGNO (reg1)]); + else + { + gcc_assert (REG_BASE_VALUE (reg1) + == static_reg_base_value[REGNO (reg1)]); + VEC_replace (rtx, reg_base_value, REGNO (reg1), + REG_BASE_VALUE (reg2)); + } +} + void end_alias_analysis (void) { --- gcc/var-tracking.c.jj 2010-11-09 13:58:30.000000000 +0100 +++ gcc/var-tracking.c 2010-11-09 19:53:52.000000000 +0100 @@ -8229,6 +8229,11 @@ vt_init_cfa_base (void) if (!MAY_HAVE_DEBUG_INSNS) return; + /* Tell alias analysis that cfa_base_rtx should temporarily share + find_base_term value with stack pointer or hard frame pointer. */ + equate_reg_base_value (cfa_base_rtx, + frame_pointer_needed + ? hard_frame_pointer_rtx : stack_pointer_rtx); val = cselib_lookup_from_insn (cfa_base_rtx, GET_MODE (cfa_base_rtx), 1, get_insns ()); preserve_value (val); @@ -8460,7 +8465,6 @@ vt_initialize (void) hard_frame_pointer_adjustment = -1; VTI (ENTRY_BLOCK_PTR)->flooded = true; vt_add_function_parameters (); - cfa_base_rtx = NULL_RTX; return true; } @@ -8503,6 +8507,10 @@ vt_finalize (void) { basic_block bb; + if (cfa_base_rtx && MAY_HAVE_DEBUG_INSNS) + equate_reg_base_value (cfa_base_rtx, cfa_base_rtx); + cfa_base_rtx = NULL_RTX; + FOR_EACH_BB (bb) { VEC_free (micro_operation, heap, VTI (bb)->mos); --- gcc/testsuite/gcc.dg/pr46387.c.jj 2010-11-09 20:35:53.000000000 +0100 +++ gcc/testsuite/gcc.dg/pr46387.c 2010-11-09 20:35:44.000000000 +0100 @@ -0,0 +1,32 @@ +/* PR debug/46387 */ +/* { dg-do compile } */ +/* { dg-options "-g -O2" } */ + +struct S { double x; double y; short z; }; +int a = 0, b = 0, c; +void bar (int, int, int); +void baz (int *, int *, int *); + +void +foo (struct S *v) +{ + int x, y, z; + if (!a && b != 0) + return; + if (v->z) + baz (&x, &y, &z); + else + { + x = v->x; + y = v->y; + } + x = x / (5 + 1); + y = y / (5 + 1); + if (x < 0) + x = 0; + if (x > c - 1) + x = c - 1; + if (b == 0) + bar (x, y, 1); + return; +}
Hi! On IA-64 the attached testcase ICEs, because a MEM isn't properly invalidated in cselib which causes ICE when handling a COND_EXEC storing to that MEM. The problem is in the {sp,hfp}+off -> {cfa_base_rtx}+off2 replacements done by var-tracking, although the whole IL is transformed that way, on the side stuff computed by init_alias_analysis is not, and as alias analysis normally expects that argp based memory can't alias with sp based memory or hfp based memory (which is certainly true before reload and after reload argp shouldn't be normally used in the IL), find_base_term can be different between the hard reg containing some sp (or hfp) based address and VALUE corresponding to it. Fixed by telling (temporarily) alias.c that argp (resp. fp) can during var-tracking alias with sp (resp. hfp) based addresses. Bootstrapped/regtested on x86_64-linux and i686-linux and tested on the testcase with ia64-linux cross. Ok for trunk? 2010-11-09 Jakub Jelinek <jakub@redhat.com> PR debug/46387 * rtl.h (equate_reg_base_value): New prototype. * alias.c (equate_reg_base_value): New function. * var-tracking.c (vt_init_cfa_base): Use it. (vt_finalize): Likewise. Clear cfa_base_rtx here... (vt_initialize): ... instead of here. * gcc.dg/pr46387.c: New test. Jakub