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. 23, 2010, 10:45 p.m.
Message ID <AANLkTikFGPhStaeCe6J_=Kd8z3_7qM95V8y5=KWM5+Y5@mail.gmail.com>
Download mbox | patch
Permalink /patch/69020/
State New
Headers show

Comments

H.J. Lu - Oct. 23, 2010, 10:45 p.m.
On Sat, Oct 23, 2010 at 1:42 PM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> 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.

This is the updated patch. However, according to

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45865#c13

the problem is deeper than that.  Revision 164552:

http://gcc.gnu.org/ml/gcc-cvs/2010-09/msg00849.html

also has caused bootstrap failures on Solaris/x86 in addition
to PR 45865. Should it be reverted for now?

Thanks.

Patch

diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c
index a19ba8d..e5e1775 100644
--- a/gcc/cfgrtl.c
+++ b/gcc/cfgrtl.c
@@ -1259,9 +1259,20 @@  force_nonfallthru_and_redirect (edge e, basic_block targe
t)
   else
     {
       rtx label = block_label (target);
-      emit_jump_insn_after_setloc (gen_jump (label), BB_END (jump_block), loc);
+      rtx jump = emit_jump_insn_after_setloc (gen_jump (label),
+					      BB_END (jump_block), loc);
       JUMP_LABEL (BB_END (jump_block)) = label;
       LABEL_NUSES (label)++;
+
+      /* JUMP is in an epilogue, mark it as return jump.  */
+      FOR_BB_INSNS (jump_block, note)
+	if (NOTE_P (note)
+	    && NOTE_KIND (note) == NOTE_INSN_EPILOGUE_BEG)
+	  {
+	    jump = pc_set (jump);
+	    SET_IS_RETURN_P (jump) = 1;
+	    break;
+	  }
     }