On 28/07/11 11:35, Bernd Schmidt wrote: > On 07/21/11 11:52, Richard Sandiford wrote: >> The name "active_insn_after" seems a bit too similar to "next_active_insn" >> for the difference to be obvious. How about something like >> "first_active_target_insn" instead? > > Changed. >>> - for (; insn; insn = next) >>> + for (; insn && !ANY_RETURN_P (insn); insn = next) >>> { >>> if (NONJUMP_INSN_P (insn) && GET_CODE (PATTERN (insn)) == SEQUENCE) >>> insn = XVECEXP (PATTERN (insn), 0, 0); >> >> Since ANY_RETURN looks for patterns, while this loop iterates over insns, >> I think it'd be more obvious to have: >> >> if (insn && ANY_RETURN_P (insn)) >> return 1; >> >> above the loop instead > > That alone wouldn't work since we assign JUMP_LABELs to next. Left alone > for now. > >>> --- gcc/jump.c (revision 176230) >>> +++ gcc/jump.c (working copy) >>> @@ -1217,7 +1217,7 @@ delete_related_insns (rtx insn) >> >> Given what you said above, and given that this is a public function, >> I think we should keep the null check. > > Changed. >> >> This pattern came up in reorg.c too, so maybe it would be worth having >> a jump_to_label_p inline function somewhere, such as: > > Done. Only has two uses for now though; reorg.c uses different patterns > mostly. > >> It looks like the old code tried to allow returns to be redirected >> to a label -- (return) to (set (pc) (label_ref)) -- whereas the new >> code doesn't. [...] >> >> How about: >> >> x = redirect_target (nlabel); >> if (GET_CODE (x) == LABEL_REF && loc == &PATTERN (insn)) >> x = gen_rtx_SET (VOIDmode, pc_rtx, x); >> validate_change (insn, loc, x, 1); > > Changed, although this probably isn't a useful thing to allow (it will > just add one more unnecessary jump to the code?). > > [ifcvt changes] >> I found the placement of this code a bit confusing as things stand. >> new_dest_label is only meaningful if other_bb != new_dest, so it seemed >> like something that should directly replace the existing new_label >> assignment. It's OK if it makes the shrink-wrap stuff easier though. > > Changed. > >>> @@ -1195,6 +1195,9 @@ duplicate_insn_chain (rtx from, rtx to) >>> break; >>> } >>> copy = emit_copy_of_insn_after (insn, get_last_insn ()); >>> + if (JUMP_P (insn) && JUMP_LABEL (insn) != NULL_RTX >>> + && ANY_RETURN_P (JUMP_LABEL (insn))) >>> + JUMP_LABEL (copy) = JUMP_LABEL (insn); >> >> I think this should go in emit_copy_of_insn_after instead. > > Here I'd like to avoid modifying the existing code in > emit_copy_of_insn_after if possible. Not sure why it's not copying > JUMP_LABELS, but that's something I'd prefer to investigate at some > other time rather than risk breaking things. > >>> @@ -2294,6 +2294,8 @@ create_cfi_notes (void) >>> dwarf2out_frame_debug (insn, false); >>> continue; >>> } >>> + if (GET_CODE (pat) == ADDR_VEC || GET_CODE (pat) == ADDR_DIFF_VEC) >>> + continue; >>> >>> if (GET_CODE (pat) == SEQUENCE) >>> { >> >> rth better approve this bit... > > It went away. > > New patch below. Retested on i686-linux and mips64-elf. Ok? > > > Bernd
This causes http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49891 R.