> Am 18.09.2018 um 02:57 schrieb Jeff Law <[email protected]>:
>
> On 9/5/18 2:34 AM, Ilya Leoshkevich wrote:
>> --- 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).
Yes, it’s RETURN+USE. I allowed CLOBBERs, because bb_is_just_return
already allows them, but I don’t think it's necessary for the S/390 use
case, so I can make it more restrictive if needed.
>
> 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?
The original RTL has the following form:
(jump_insn 1 (set %pc (if_then_else (ne %cc 0) (label_ref 2) %pc)))
...
(code_label 2)
(jump_insn 3 (parallel [(return) (use %r14)]))
and the goal is to transform (jump_insn 1) to:
(jump_insn 1
(parallel
[(set %pc (if_then_else (ne %cc 0) (return) %pc))
(use %r14)]))
while keeping (code_label 2) and (jump_insn 3) intact. So I have to
create a new PARALLEL based on the original one.
>
>
>> 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?
Right, this is related. This is to indicate to copy_update_parallel,
which of the side-effects need to be updated.
