Re: [PATCH] S/390: Fix conditional returns

[Jeff Law <law at redhat dot com>]

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  <iii@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  <iii@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