On 9/5/18 2:34 AM, Ilya Leoshkevich wrote:
> S/390 epilogue ends with (parallel [(return) (use %r14)]) instead of
> the more usual (return) or (simple_return). This sequence is not
> recognized by the conditional return logic in try_optimize_cfg ().
>
> gcc/ChangeLog:
>
> 2018-08-28 Ilya Leoshkevich <i...@linux.ibm.com>
>
> PR target/80080
> * cfgcleanup.c (bb_is_just_return): Accept PARALLELs containing
> RETURNs.
> * cfgrtl.c (rtl_verify_bb_layout): Handle PARALLELs containing
> conditional jumps.
> * config/s390/s390.md: Recognize PARALLELs containing RETURNs.
> * jump.c (copy_update_parallel): Create a copy of a PARALLEL
> in which one of side effects is replaced.
> (redirect_exp_1): Handle jump targets that are PARALLELs
> containing RETURNs.
> (redirect_jump_2): Likewise.
> (return_in_parallel): Recognize PARALLELs containing RETURNs.
> * rtl.h (return_in_parallel): Add declaration.
>
> gcc/testsuite/ChangeLog:
>
> 2018-08-28 Ilya Leoshkevich <i...@linux.ibm.com>
>
> PR target/80080
> * gcc.target/s390/risbg-ll-3.c: Expect conditional returns.
> * gcc.target/s390/zvector/vec-cmp-2.c: Likewise.
> ---
> gcc/cfgcleanup.c | 2 +-
> gcc/cfgrtl.c | 3 +-
> gcc/config/s390/s390.md | 13 +++-
> gcc/jump.c | 69 ++++++++++++++++++-
> gcc/rtl.h | 1 +
> gcc/testsuite/gcc.target/s390/risbg-ll-3.c | 4 +-
> .../gcc.target/s390/zvector/vec-cmp-2.c | 48 ++++++-------
> 7 files changed, 108 insertions(+), 32 deletions(-)
>
> diff --git a/gcc/cfgcleanup.c b/gcc/cfgcleanup.c
> index 4a5dc29d14f..7f2545f453f 100644
> --- a/gcc/cfgcleanup.c
> +++ b/gcc/cfgcleanup.c
> @@ -2624,7 +2624,7 @@ bb_is_just_return (basic_block bb, rtx_insn **ret,
> rtx_insn **use)
> {
> rtx pat = PATTERN (insn);
>
> - if (!*ret && ANY_RETURN_P (pat))
> + if (!*ret && (ANY_RETURN_P (pat) || return_in_parallel (pat)))
> *ret = insn;
> else if (!*ret && !*use && GET_CODE (pat) == USE
> && REG_P (XEXP (pat, 0))
So what else is in the return insn that requires you to test for
return_in_parallel? If we're going to allow a return in a parallel,
here I think we need to tighten down its form given the intended ways
bb_is_just_return is supposed to be used. Essentially other side
effects would seem to be forbidden in the parallel. ie, you could have
a PARALLEL with a return and use inside, but not a return with anything
else inside (such as a clobber).
Why do you need to make a copy of the parallel RTX in redirect_exp_1?
We don't do it for other cases -- why can't we just use validate_change
like all the other RTXs?
> /* Throughout LOC, redirect OLABEL to NLABEL. Treat null OLABEL or
> NLABEL as a return. Accrue modifications into the change group. */
>
> @@ -1437,9 +1457,22 @@ redirect_exp_1 (rtx *loc, rtx olabel, rtx nlabel,
> rtx_insn *insn)
> if ((code == LABEL_REF && label_ref_label (x) == olabel)
> || x == olabel)
> {
> - x = redirect_target (nlabel);
> - if (GET_CODE (x) == LABEL_REF && loc == &PATTERN (insn))
> - x = gen_rtx_SET (pc_rtx, x);
> + rtx *nret = return_in_parallel (nlabel);
> +
> + if (nret)
> + {
> + rtx npat;
> +
> + x = *nret;
> + npat = copy_update_parallel (nlabel, nret, PATTERN (insn));
> + validate_change (insn, &PATTERN (insn), npat, 1);
> + }
> + else
> + {
> + x = redirect_target (nlabel);
> + if (GET_CODE (x) == LABEL_REF && loc == &PATTERN (insn))
> + x = gen_rtx_SET (pc_rtx, x);
> + }
> validate_change (insn, loc, x, 1);
Why the need to use copy_update_parallel here? Is there a reason why
validate_change is insufficient?
> return;
> }
> @@ -1551,10 +1584,15 @@ void
> redirect_jump_2 (rtx_jump_insn *jump, rtx olabel, rtx nlabel, int
> delete_unused,
> int invert)
> {
> + rtx *ret;
> rtx note;
>
> gcc_assert (JUMP_LABEL (jump) == olabel);
>
> + ret = return_in_parallel (nlabel);
> + if (ret)
> + nlabel = *ret;
Why does return_in_parallel return an rtx *? Can't you just return the
rtx and avoid the unnecessary dereferencing? I guess this ultimately
comes back to why can't you use validate_change like everyone else in
redirect_exp_1?
jeff
Jeff
