| Message ID | 4F79BE19.8050905@ispras.ru |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 02, 2012 at 06:56:25PM +0400, Andrey Belevantsev wrote: > After Richi's RTL generation related cleanups went it, the extra > cleanup_cfg call was added so we are no longer lucky to have the > proper fallthru edge in this case. The PR trail has the patch to > purge_dead_edges making it consider this situation, but I still > prefer the below patch that fixes only the invalid asm case. The > reason is that I think it unlikely that after initial RTL expansion > (of which the instantiate virtual regs pass seems to be the part) we > will get the problematic situation. However, I'm happy to test the > PR trail patch, too. I don't like blindly changing edge into FALLTHRU, generally the edge could be abnormal, or EH, etc. and making that fallthru is not a good idea. I'd prefer if wherever the fallthru edge is removed the other normal edge(s) are adjusted. Jakub
On 03.04.2012 13:36, Jakub Jelinek wrote: > On Mon, Apr 02, 2012 at 06:56:25PM +0400, Andrey Belevantsev wrote: >> After Richi's RTL generation related cleanups went it, the extra >> cleanup_cfg call was added so we are no longer lucky to have the >> proper fallthru edge in this case. The PR trail has the patch to >> purge_dead_edges making it consider this situation, but I still >> prefer the below patch that fixes only the invalid asm case. The >> reason is that I think it unlikely that after initial RTL expansion >> (of which the instantiate virtual regs pass seems to be the part) we >> will get the problematic situation. However, I'm happy to test the >> PR trail patch, too. > > I don't like blindly changing edge into FALLTHRU, generally the edge > could be abnormal, or EH, etc. and making that fallthru is not a good idea. > I'd prefer if wherever the fallthru edge is removed the other normal edge(s) > are adjusted. Well, as I mentioned in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51106#c18 the removal happens in try_optimize_cfg around line 2617, that's the code that deals with removing trivially empty block (in the PR case, the succ of the asm goto block is trivially empty). After that we have an asm goto acting as an unconditional jump with a single succ edge and no fallthru bit, which seems perfectly fine. We get into trouble when we actually realize that the asm is bogus. Thus I've tried to fix it up at this time. The options that we briefly discussed on IRC with Richi are as follows: - Fix up try_optimize_cfg in the case of asm gotos, but it is not clear to me how do we do this -- we don't yet distinguish between good and bad asm goto at this point; - Fix up function.c as I did but make it more robust. Either handle more cases with strange edges (EH seems also possible after introducing the throw attribute for asms), or just remove the asm and insert the unconditional jump pointing to the place where the asm was, retaining all the flags; - Fix up purge_dead_edges. as I did initially in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=51106#c16. When we find a situation of no jump at the end of block and no fallthru edge, assert this happens only with single_succ_p and actually make this edge fallthru. (Probably also watch out whether we need to make it fake or whatever.) Or as Richi tried, just accept the situation of having no successors here, as in -- no fallthru edge on entry to purge_dead_edges and no jump means no successors, period. I think that just nobody deleted unconditional jumps with delete_insn_and_edges previously, otherwise I don't understand why this did not trigger. Thoughts? Andrey > > Jakub
diff --git a/gcc/function.c b/gcc/function.c index 3e903ef..a2638bb 100644 --- a/gcc/function.c +++ b/gcc/function.c @@ -1730,6 +1730,15 @@ instantiate_virtual_regs_in_insn (rtx insn) if (!check_asm_operands (PATTERN (insn))) { error_for_asm (insn, "impossible constraint in %<asm%>"); + if (JUMP_P (insn)) + { + basic_block bb = BLOCK_FOR_INSN (insn); + edge e; + + if (single_succ_p (bb) + && !((e = single_succ_edge (bb))->flags & EDGE_FALLTHRU)) + e->flags |= EDGE_FALLTHRU; + } delete_insn_and_edges (insn); } }