| Message ID | 5593F08D.6090107@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
On 7/1/15 21:52, Bernd Schmidt wrote: > On 07/01/2015 03:04 AM, Chen Gang wrote: > >> For me, the more details are: >> >> - The insns have 2 loops which can be lsetup optimized. >> >> - After hwloop_optimize finishes 1st lsetup optimization, it generates >> new lsetup insn which appends to jump insn in the basic block (which >> causes the insns are not 'standard' but OK for code generation). > > The problem is that you can't append anything to a basic block after a jump. You need to create a new one. This problem doesn't usually show up since nothing ever looks at the basic block again, unless both directions from the conditional branch happen to branch to lsetup candidate loops. > OK, thanks. What you said sound reasonable to me. > Below is a patch. Can you test this with anything you have beyond the testsuite? > It can fix this issue (Bug66620), let the insns standard, and can build the bfin kernel with allmodconfig successfully (although for bfin kernel members, they stick to allmodconfig is not a good idea for bfin kernel). It finished lsetup optimization for one loop, but still left the other ( get the same .s as my original fix). for 2nd times in hwloop_optimize, it return false. And welcome any additional ideas for it. For me, my original fix is incorrect: it still remains the insns in the incorrect state (although my fix can generate the correct .s, and can build bfin kernel with allmodconfig successfully). Thanks.
On 07/01/2015 11:27 PM, Chen Gang wrote: > On 7/1/15 21:52, Bernd Schmidt wrote: >> On 07/01/2015 03:04 AM, Chen Gang wrote: >> >>> For me, the more details are: >>> >>> - The insns have 2 loops which can be lsetup optimized. >>> >>> - After hwloop_optimize finishes 1st lsetup optimization, it generates >>> new lsetup insn which appends to jump insn in the basic block (which >>> causes the insns are not 'standard' but OK for code generation). >> >> The problem is that you can't append anything to a basic block after a jump. You need to create a new one. This problem doesn't usually show up since nothing ever looks at the basic block again, unless both directions from the conditional branch happen to branch to lsetup candidate loops. >> > > OK, thanks. What you said sound reasonable to me. > >> Below is a patch. Can you test this with anything you have beyond the testsuite? >> > > It can fix this issue (Bug66620), let the insns standard, and can build > the bfin kernel with allmodconfig successfully (although for bfin kernel > members, they stick to allmodconfig is not a good idea for bfin kernel). > > It finished lsetup optimization for one loop, but still left the other ( > get the same .s as my original fix). for 2nd times in hwloop_optimize, it > return false. And welcome any additional ideas for it. > I shall continue to analyse why 2nd lsetup optimiation has not happened. Hope I can finish within next week (2015-07-12). > For me, my original fix is incorrect: it still remains the insns in the > incorrect state (although my fix can generate the correct .s, and can > build bfin kernel with allmodconfig successfully). > > > Thanks. >
On 07/03/2015 04:13 AM, Chen Gang wrote: > On 07/01/2015 11:27 PM, Chen Gang wrote: >> On 7/1/15 21:52, Bernd Schmidt wrote: >>> Below is a patch. Can you test this with anything you have beyond the testsuite? >>> >> >> It can fix this issue (Bug66620), let the insns standard, and can build >> the bfin kernel with allmodconfig successfully (although for bfin kernel >> members, they stick to allmodconfig is not a good idea for bfin kernel). >> >> It finished lsetup optimization for one loop, but still left the other ( >> get the same .s as my original fix). for 2nd times in hwloop_optimize, it >> return false. And welcome any additional ideas for it. >> > > I shall continue to analyse why 2nd lsetup optimiation has not happened. > Hope I can finish within next week (2015-07-12). I've committed my patch after testing bfin-elf. There's no great mystery why the second optimization doesn't happen: the point where it thinks it has to insert the LSETUP is after the loop, and the instruction doesn't allow that. Possibly we could change that - when the loop is entered at the top but not through a fallthrough edge, we could make a new block ahead of it and put the LSETUP in there. Bernd
On 7/6/15 20:51, Bernd Schmidt wrote: > On 07/03/2015 04:13 AM, Chen Gang wrote: >> On 07/01/2015 11:27 PM, Chen Gang wrote: >>> On 7/1/15 21:52, Bernd Schmidt wrote: >>>> Below is a patch. Can you test this with anything you have beyond the testsuite? >>>> >>> >>> It can fix this issue (Bug66620), let the insns standard, and can build >>> the bfin kernel with allmodconfig successfully (although for bfin kernel >>> members, they stick to allmodconfig is not a good idea for bfin kernel). >>> >>> It finished lsetup optimization for one loop, but still left the other ( >>> get the same .s as my original fix). for 2nd times in hwloop_optimize, it >>> return false. And welcome any additional ideas for it. >>> >> >> I shall continue to analyse why 2nd lsetup optimiation has not happened. >> Hope I can finish within next week (2015-07-12). > > I've committed my patch after testing bfin-elf. There's no great mystery why the second optimization doesn't happen: the point where it thinks it has to insert the LSETUP is after the loop, and the instruction doesn't allow that. Possibly we could change that - when the loop is entered at the top but not through a fallthrough edge, we could make a new block ahead of it and put the LSETUP in there. > OK, thanks. for me, the fix is enough for this issue. And need we add the related .i file to testsuite, too? And thank you for your information, I shall try to let 2nd times lsetup have effect in another patch, hope I can succeed :-). Thanks
diff --git a/gcc/config/bfin/bfin.c b/gcc/config/bfin/bfin.c index 8c1e18a..2c6f195 100644 --- a/gcc/config/bfin/bfin.c +++ b/gcc/config/bfin/bfin.c @@ -3796,8 +3796,19 @@ hwloop_optimize (hwloop_info loop) { gcc_assert (JUMP_P (prev)); prev = PREV_INSN (prev); + emit_insn_after (seq, prev); + } + else + { + emit_insn_after (seq, prev); + BB_END (loop->incoming_src) = prev; + basic_block new_bb = create_basic_block (seq, seq_end, + loop->head->prev_bb); + edge e = loop->incoming->last (); + gcc_assert (e->flags & EDGE_FALLTHRU); + redirect_edge_succ (e, new_bb); + make_edge (new_bb, loop->head, 0); } - emit_insn_after (seq, prev); } else {