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

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.

-- 
H.J.
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;
+	  }
     }

   emit_barrier_after (BB_END (jump_block));