Patchwork RFC: PATCH: PR rtl-optimization/45865: [4.6 Regression] ifcvt/crossjump failed to mark return jump

login
register
mail settings
Submitter H.J. Lu
Date Oct. 21, 2010, 5:46 p.m.
Message ID <AANLkTinTyYkLwph+XHwV_N=MJspK-zSNnBno9hSihj6d@mail.gmail.com>
Download mbox | patch
Permalink /patch/68731/
State New
Headers show

Comments

H.J. Lu - Oct. 21, 2010, 5:46 p.m.
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?

Thanks.
Steven Bosscher - Oct. 23, 2010, 2:17 p.m.
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
H.J. Lu - Oct. 23, 2010, 4:16 p.m.
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.
Steven Bosscher - Oct. 23, 2010, 8:42 p.m.
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

Patch

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;
+}