| Message ID | 201110151621.27366.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
On 10/15/11 16:21, Eric Botcazou wrote: > so the correct fix is very likely something like: > > Index: cfgrtl.c > =================================================================== > --- cfgrtl.c (revision 179844) > +++ cfgrtl.c (working copy) > @@ -1024,13 +1024,20 @@ patch_jump_insn (rtx insn, rtx old_label > > if (!currently_expanding_to_rtl || JUMP_LABEL (insn) == old_label) > { > + rtx new_label; > + > /* If the insn doesn't go where we think, we're confused. */ > gcc_assert (JUMP_LABEL (insn) == old_label); > > + if (new_bb == EXIT_BLOCK_PTR) > + new_label = ret_rtx; > + else > + new_label = block_label (new_bb); > + > /* If the substitution doesn't succeed, die. This can happen > if the back end emitted unrecognizable instructions or if > target is exit block on some arches. */ > - if (!redirect_jump (insn, block_label (new_bb), 0)) > + if (!redirect_jump (insn, new_label, 0)) > { > gcc_assert (new_bb == EXIT_BLOCK_PTR); > return false; > > > Bernd, should all the callers of redirect_jump/redirect_jump_1 be audited for > this pattern (there are 3 of them in cfgrtl.c for example)? I think first we'll need to find the caller and make sure it really wants a return and not a simple_return. Bernd
On 10/15/11 16:21, Eric Botcazou wrote: >> PR middle-end/50496 >> * cfgrtl.c (try_redirect_by_replacing_jump): Treat EXIT_BLOCK_PTR case >> separately before call to redirect_jump(). Add assertion. >> (patch_jump_insn): Same. > > This will definitely disable redirections to the exit block. Yes. However, in the case that caused the PR, this was attempted with reload_completed == 0, so we cannot generate return patterns anyway and must fail. I think the original patch is OK. Bernd
> Yes. However, in the case that caused the PR, this was attempted with > reload_completed == 0, so we cannot generate return patterns anyway and > must fail. OK, but we clean up the CFG after reload is completed (e.g. just before emitting the prologue) so in this case we can generate return patterns.
On 10/17/11 19:43, Eric Botcazou wrote: >> Yes. However, in the case that caused the PR, this was attempted with >> reload_completed == 0, so we cannot generate return patterns anyway and >> must fail. > > OK, but we clean up the CFG after reload is completed (e.g. just before > emitting the prologue) so in this case we can generate return patterns. thread_prologue_and_epilogue_insns should detect all cases where a return insn can be created. So any CFG cleanup that runs before it does not need this functionality. Bernd
> thread_prologue_and_epilogue_insns should detect all cases where a > return insn can be created. So any CFG cleanup that runs before it does > not need this functionality. So we're left with CFG cleanups that run after it and could forward edges to an edge from a return insn to the exit block in order to build a new return insn.
On 2011/10/18 04:03 PM, Eric Botcazou wrote: >> thread_prologue_and_epilogue_insns should detect all cases where a >> return insn can be created. So any CFG cleanup that runs before it does >> not need this functionality. > > So we're left with CFG cleanups that run after it and could forward edges to an > edge from a return insn to the exit block in order to build a new return insn. Bernd, why can't we simply remove the assertion? The pre-reload case will fail at validation and return 0, matching pre-reload, pre-shrink-wrap behavior, while any possible remaining post-reload redirection to the exit block can just use 'ret_rtx' as the rare fallback (I see you have retained the NULL case in redirect_target()) Chung-Lin
On 10/24/11 20:02, Chung-Lin Tang wrote: > On 2011/10/18 04:03 PM, Eric Botcazou wrote: >>> thread_prologue_and_epilogue_insns should detect all cases where a >>> return insn can be created. So any CFG cleanup that runs before it does >>> not need this functionality. >> >> So we're left with CFG cleanups that run after it and could forward edges to an >> edge from a return insn to the exit block in order to build a new return insn. We have no testcases to suggest that this ever happens. > Bernd, why can't we simply remove the assertion? The pre-reload case > will fail at validation and return 0, matching pre-reload, > pre-shrink-wrap behavior, while any possible remaining post-reload > redirection to the exit block can just use 'ret_rtx' as the rare > fallback No, after prologue insertion we have to distinguish between ret_rtx and simple_return_rtx. > (I see you have retained the NULL case in redirect_target()) That may just be a thinko. Bernd
Index: cfgrtl.c =================================================================== --- cfgrtl.c (revision 179844) +++ cfgrtl.c (working copy) @@ -1024,13 +1024,20 @@ patch_jump_insn (rtx insn, rtx old_label if (!currently_expanding_to_rtl || JUMP_LABEL (insn) == old_label) { + rtx new_label; + /* If the insn doesn't go where we think, we're confused. */ gcc_assert (JUMP_LABEL (insn) == old_label); + if (new_bb == EXIT_BLOCK_PTR) + new_label = ret_rtx; + else + new_label = block_label (new_bb); + /* If the substitution doesn't succeed, die. This can happen if the back end emitted unrecognizable instructions or if target is exit block on some arches. */ - if (!redirect_jump (insn, block_label (new_bb), 0)) + if (!redirect_jump (insn, new_label, 0)) { gcc_assert (new_bb == EXIT_BLOCK_PTR); return false;