| Message ID | AANLkTinTyYkLwph+XHwV_N=MJspK-zSNnBno9hSihj6d@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Oct 21, 2010 at 7:46 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Sun, Oct 10, 2010 at 6:50 AM, Jason Merrill <jason@redhat.com> wrote: >> >> If the problem is that there's a return jump that isn't so marked, that >> seems to be the thing to fix, rather than assume that any jump within an >> epilogue is a return. >> >> Jason >> > > Does this patch make senses? I'd like to know where the unmarked jump is created, and mark it there. Ciao! Steven
On Sat, Oct 23, 2010 at 7:17 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: > On Thu, Oct 21, 2010 at 7:46 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> On Sun, Oct 10, 2010 at 6:50 AM, Jason Merrill <jason@redhat.com> wrote: >>> >>> If the problem is that there's a return jump that isn't so marked, that >>> seems to be the thing to fix, rather than assume that any jump within an >>> epilogue is a return. >>> >>> Jason >>> >> >> Does this patch make senses? > > I'd like to know where the unmarked jump is created, and mark it there. > The unmarked jump is created by force_nonfallthru_and_redirect: if (e->goto_locus && e->goto_block == NULL) loc = e->goto_locus; else loc = 0; e->flags &= ~EDGE_FALLTHRU; if (target == EXIT_BLOCK_PTR) { #ifdef HAVE_return emit_jump_insn_after_setloc (gen_return (), BB_END (jump_block), loc); #else gcc_unreachable (); #endif } else { rtx label = block_label (target); emit_jump_insn_after_setloc (gen_jump (label), BB_END (jump_block), loc); JUMP_LABEL (BB_END (jump_block)) = label; LABEL_NUSES (label)++; } My patch adds: /* If jump_block has an epilogue, mark the last insn as return jump if needed. */ FOR_BB_INSNS (jump_block, note) if (NOTE_P (note) && NOTE_KIND (note) == NOTE_INSN_EPILOGUE_BEG) { rtx jump = BB_END (jump_block); if (!returnjump_p (jump)) { jump = pc_set (jump); SET_IS_RETURN_P (jump) = 1; } break; } immediately after the jump is created.
On Sat, Oct 23, 2010 at 6:16 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Sat, Oct 23, 2010 at 7:17 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote: >> On Thu, Oct 21, 2010 at 7:46 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >>> On Sun, Oct 10, 2010 at 6:50 AM, Jason Merrill <jason@redhat.com> wrote: >>>> >>>> If the problem is that there's a return jump that isn't so marked, that >>>> seems to be the thing to fix, rather than assume that any jump within an >>>> epilogue is a return. >>>> >>>> Jason >>>> >>> >>> Does this patch make senses? >> >> I'd like to know where the unmarked jump is created, and mark it there. >> > > The unmarked jump is created by force_nonfallthru_and_redirect: OK, and I suppose it is generated by the line with gen_return? > if (e->goto_locus && e->goto_block == NULL) > loc = e->goto_locus; > else > loc = 0; > e->flags &= ~EDGE_FALLTHRU; > if (target == EXIT_BLOCK_PTR) > { > #ifdef HAVE_return > emit_jump_insn_after_setloc (gen_return (), BB_END (jump_block), loc); i.e. here? > #else > gcc_unreachable (); > #endif > } > else > { > rtx label = block_label (target); > emit_jump_insn_after_setloc (gen_jump (label), BB_END (jump_block), loc); or here? > JUMP_LABEL (BB_END (jump_block)) = label; > LABEL_NUSES (label)++; > } > > My patch adds: > > /* If jump_block has an epilogue, mark the last insn as return jump > if needed. */ > FOR_BB_INSNS (jump_block, note) > if (NOTE_P (note) > && NOTE_KIND (note) == NOTE_INSN_EPILOGUE_BEG) > { > rtx jump = BB_END (jump_block); > if (!returnjump_p (jump)) > { > jump = pc_set (jump); > SET_IS_RETURN_P (jump) = 1; > } > break; > } > > immediately after the jump is created. If jump comes from gen_return, you don't need to loop over all insns of the jump_block to see if jump should have IS_RETURN_P set. (And if jump comes from the simple gen_jump, you could look for a note there -- but I do hope your return jump goes to EXIT_BLOCK_PTR.). Anyway, there is one more important thing I don't understand about this patch: I can't find any place (with grep) where any code has to touch SET_IS_RETURN_P, except the check in returnjump_p_1. Unless I'm overlooking something, SET_IS_RETURN_P is read-only at the moment. So you are probably papering over a deeper bug than just a return not marked. Ciao! Steven
diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c index e46050d..ccc9e29 100644 --- a/gcc/cfgrtl.c +++ b/gcc/cfgrtl.c @@ -1255,6 +1255,21 @@ force_nonfallthru_and_redirect (edge e, basic_block target) LABEL_NUSES (label)++; } + /* If jump_block has an epilogue, mark the last insn as return jump + if needed. */ + FOR_BB_INSNS (jump_block, note) + if (NOTE_P (note) + && NOTE_KIND (note) == NOTE_INSN_EPILOGUE_BEG) + { + rtx jump = BB_END (jump_block); + if (!returnjump_p (jump)) + { + jump = pc_set (jump); + SET_IS_RETURN_P (jump) = 1; + } + break; + } + emit_barrier_after (BB_END (jump_block)); redirect_edge_succ_nodup (e, target); --- /dev/null 2010-09-09 09:16:30.485584932 -0700 +++ gcc/gcc/testsuite/gcc.dg/pr45865.c 2010-10-09 19:11:53.988124142 -0700 @@ -0,0 +1,28 @@ +/* PR debug/45865 */ +/* { dg-do compile } */ +/* { dg-options "-O2 -fcompare-debug" } */ + +typedef union tree_node *tree; +enum ix86_builtin_type { + IX86_BT_LAST_VECT, + IX86_BT_LAST_PTR +}; +extern const enum ix86_builtin_type ix86_builtin_type_ptr_base[]; +extern tree build_qualified_type (tree, int); +extern tree build_pointer_type (tree); +tree +ix86_get_builtin_type (enum ix86_builtin_type tcode, unsigned int index) +{ + tree type, itype; + int quals; + if (tcode <= IX86_BT_LAST_PTR) + quals = 0x0; + else + quals = 0x1; + itype = ix86_get_builtin_type (ix86_builtin_type_ptr_base[index], + index); + if (quals != 0x0) + itype = build_qualified_type (itype, quals); + type = build_pointer_type (itype); + return type; +} --- /dev/null 2010-09-09 09:16:30.485584932 -0700 +++ gcc/gcc/testsuite/gcc.dg/torture/pr45865.c 2010-10-05 19:36:04.918278945 -0700 @@ -0,0 +1,54 @@ +/* { dg-do compile } */ + +typedef struct rtx_def *rtx; +enum machine_mode { + VOIDmode, + CCFPmode, + CCFPUmode, + MAX_MACHINE_MODE +}; +enum mode_class { + MODE_CC, + MODE_FLOAT, + MODE_COMPLEX_FLOAT, + MODE_VECTOR_FLOAT +}; +extern const enum mode_class mode_class[(int) MAX_MACHINE_MODE]; +enum rtx_code { + UNKNOWN, + GEU, + ORDERED, + CONST_INT +}; +struct rtx_def { + unsigned int code: 16; + unsigned int mode : 8; +}; +extern enum rtx_code reverse_condition (enum rtx_code); +enum rtx_code +reversed_comparison_code_parts (enum rtx_code code, rtx insn, rtx arg0, + rtx arg1) +{ + enum machine_mode mode; + mode = (enum machine_mode) (arg0)->mode; + if (mode == VOIDmode) + mode = (enum machine_mode) (arg1)->mode; + if ((mode_class[(int) (mode)]) == MODE_CC) + return (mode != CCFPmode && mode != CCFPUmode + ? reverse_condition (code) + : reverse_condition_maybe_unordered (code)); + switch (code) + { + case GEU: + return reverse_condition (code); + case ORDERED: + return UNKNOWN; + } + if (((enum rtx_code) (arg0)->code) == CONST_INT + || (((enum machine_mode) (arg0)->mode) != VOIDmode + && ! ((mode_class[(int) (mode)]) == MODE_FLOAT + || (mode_class[(int) (mode)]) == MODE_COMPLEX_FLOAT + || (mode_class[(int) (mode)]) == MODE_VECTOR_FLOAT))) + return reverse_condition (code); + return UNKNOWN; +}