[PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Chen Gang]

For bfin looping optimization, after lsetup optimization, it can have
the correct lsetup related insns which causes gcc_assert for jump_insn.

The related bug is Bug 66620.

2015-06-28  Chen Gang  <gang.chen.5i5j@gmail.com>

	* config/bfin/bfin.c (hwloop_optimize): Use return false instead
	of gcc_assert for checking jump_insn.
---
 gcc/config/bfin/bfin.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/gcc/config/bfin/bfin.c b/gcc/config/bfin/bfin.c
index 3b4b54e..91866dd 100644
--- a/gcc/config/bfin/bfin.c
+++ b/gcc/config/bfin/bfin.c
@@ -3520,7 +3520,13 @@ hwloop_optimize (hwloop_info loop)
       if (vec_safe_length (loop->incoming) > 1
 	  || !(loop->incoming->last ()->flags & EDGE_FALLTHRU))
 	{
-	  gcc_assert (JUMP_P (insn));
+	  if (!JUMP_P (insn))
+	    {
+	      if (dump_file)
+		fprintf (dump_file, ";; loop %d lsetup may already inserted\n",
+			 loop->loop_no);
+	      return false;
+	    }
 	  insn = PREV_INSN (insn);
 	}
 
-- 
1.9.3

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Bernd Schmidt]

On 06/28/2015 04:15 PM, Chen Gang wrote:
> For bfin looping optimization, after lsetup optimization, it can have
> the correct lsetup related insns which causes gcc_assert for jump_insn.

I've been debugging this for a bit, and at least the explanation of the 
patch is wrong - it's finding an LSETUP for a different loop. There 
seems to be an inconsistency in the CFG, and it looks like it's caused 
by the unusual (?) situation that both arms out of a conditional branch 
lead directly to a hwloop candidate.

So, not OK until further investigation I think.


Bbernd

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Chen Gang]

On 06/30/2015 03:46 AM, Bernd Schmidt wrote:
> On 06/28/2015 04:15 PM, Chen Gang wrote:
>> For bfin looping optimization, after lsetup optimization, it can have
>> the correct lsetup related insns which causes gcc_assert for jump_insn.
> 
> I've been debugging this for a bit, and at least the explanation of the
> patch is wrong - it's finding an LSETUP for a different loop. There
> seems to be an inconsistency in the CFG, and it looks like it's caused
> by the unusual (?) situation that both arms out of a conditional branch
> lead directly to a hwloop candidate.
>

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).

 - In 2nd lsetup optimization, hwloop_optimize found the insns are not
   'standard' (they are generated by hwloop_optimize itself in 1st
   lsetup optimization), so gcc_assert().

So hwloop_optimize can give up lsetup optimization for the 'unstandard'
insns, at present, all things will be OK.

If we want to try perfect, we can let hwloop_optimize can process the
'unstandard' insns for lsetup optimization. But excuse me, I am not
quite familiar with gcc internal or bfin (so I am not suitable for it).


> So, not OK until further investigation I think.
> 

If necessary (what I said above is incorrect), I shall continue to
analyse.

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 746 bytes --]

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.

Below is a patch. Can you test this with anything you have beyond the 
testsuite?


Bernd


[-- Attachment #2: bblsetup.diff --]
[-- Type: text/x-patch, Size: 707 bytes --]

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
     {

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Chen Gang]

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.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Chen Gang]

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.
> 

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Bernd Schmidt]

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

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Chen Gang]

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
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [PATCH] config/bfin/bfin.c (hwloop_optimize): Use return false instead of gcc_assert for checking jump_insn.

[Chen Gang]

On 7/7/15 06:09, Chen Gang wrote:
> On 7/6/15 20:51, Bernd Schmidt wrote:
>> On 07/03/2015 04:13 AM, Chen Gang wrote:
>>>
>>> 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.
>>

After trying, for me, we need notice about the jump insn to loop start
label, and emit lsetup insn to loop head instead of incoming_src end,
the related diff is below:


diff --git a/gcc/config/bfin/bfin.c b/gcc/config/bfin/bfin.c
index a131053..9ef2a9c 100644
--- a/gcc/config/bfin/bfin.c
+++ b/gcc/config/bfin/bfin.c
@@ -3416,6 +3416,7 @@ bfin_hardware_loop (void)
 #define MAX_LOOP_LENGTH 2042
 
 /* Maximum distance of the LSETUP instruction from the loop start.  */
+/* #define MAX_LSETUP_DISTANCE 30 */
 #define MAX_LSETUP_DISTANCE 30
 
 /* Estimate the length of INSN conservatively.  */
@@ -3456,7 +3457,7 @@ hwloop_optimize (hwloop_info loop)
   rtx seq_end;
   rtx_insn *seq;
   int length;
-  bool clobber0, clobber1;
+  bool clobber0, clobber1, direct_jmp = false;
 
   if (loop->depth > MAX_LOOP_DEPTH)
     {
@@ -3519,7 +3520,13 @@ hwloop_optimize (hwloop_info loop)
          || !(loop->incoming->last ()->flags & EDGE_FALLTHRU))
        {
          gcc_assert (JUMP_P (insn));
-         insn = PREV_INSN (insn);
+         if (JUMP_LABEL (insn) != loop->start_label)
+           insn = PREV_INSN (insn);
+         else
+           {
+             direct_jmp = true;
+             insn = loop->start_label;
+           }
        }
 
       for (; insn && insn != loop->start_label; insn = NEXT_INSN (insn))
@@ -3783,7 +3790,7 @@ hwloop_optimize (hwloop_info loop)
   seq = get_insns ();
   end_sequence ();
 
-  if (loop->incoming_src)
+  if (loop->incoming_src && !direct_jmp)
     {
       rtx_insn *prev = BB_END (loop->incoming_src);
       if (vec_safe_length (loop->incoming) > 1



Welcome any additional ideas, suggestions and completions (and I shall
send patch for it, if no additional relply within the next week).


Thanks.

> 
> 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 :-).
> 

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed