[PATCH 1/2] SH epilogue unwind, dwarf2 pass changes

[PATCH 1/2] SH epilogue unwind, dwarf2 pass changes

[Chung-Lin Tang]

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

Hi,

I found a few changes were needed to the dwarf2 pass when trying to
implement epilogue unwind for SH, mainly that the current handling of
annulled-taken branches does not seem correct; the delay slot insn
should be handled in a manner similar to an insn in the fallthru block.

Cross-tested on SH and MIPS, and bootstrapped/tested on x86_64.

Thanks,
Chung-Lin

2012-05-18  Chung-Lin Tang  <cltang@codesourcery.com>

	* dwarf2cfi.c (scan_trace): Update CFA before exiting scan.
	Handle annulled-taken branch (!INSN_FROM_TARGET_P) case.


[-- Attachment #2: dwarf2cfi.patch --]
[-- Type: text/plain, Size: 921 bytes --]

Index: dwarf2cfi.c
===================================================================
--- dwarf2cfi.c	(revision 187548)
+++ dwarf2cfi.c	(working copy)
@@ -2401,6 +2401,7 @@ scan_trace (dw_trace_info *trace)
 	{
 	  /* Propagate across fallthru edges.  */
 	  dwarf2out_flush_queued_reg_saves ();
+	  def_cfa_1 (&this_cfa);
 	  maybe_record_trace_start (insn, NULL);
 	  break;
 	}
@@ -2455,6 +2456,18 @@ scan_trace (dw_trace_info *trace)
 		  cur_cfa = &this_cfa;
 		  continue;
 		}
+	      else
+		{
+		  /* If ELT is a annulled branch-taken instruction (i.e. executed
+		     only when branch is not taken), the args_size and CFA should
+		     not change through the jump.  */
+		  create_trace_edges (control);
+
+		  /* Update and continue with the trace.  */
+		  add_cfi_insn = insn;
+		  scan_insn_after (elt);
+		  continue;
+		}
 	    }
 
 	  /* The insns in the delay slot should all be considered to happen

Re: [PATCH 1/2] SH epilogue unwind, dwarf2 pass changes

[Richard Henderson]

On 05/18/12 03:48, Chung-Lin Tang wrote:
> Hi,
> 
> I found a few changes were needed to the dwarf2 pass when trying to
> implement epilogue unwind for SH, mainly that the current handling of
> annulled-taken branches does not seem correct; the delay slot insn
> should be handled in a manner similar to an insn in the fallthru block.
> 
> Cross-tested on SH and MIPS, and bootstrapped/tested on x86_64.
> 
> Thanks,
> Chung-Lin
> 
> 2012-05-18  Chung-Lin Tang  <cltang@codesourcery.com>
> 
> 	* dwarf2cfi.c (scan_trace): Update CFA before exiting scan.
> 	Handle annulled-taken branch (!INSN_FROM_TARGET_P) case.
> 

Do you have a test case for this?


r~

Re: [PATCH 1/2] SH epilogue unwind, dwarf2 pass changes

[Richard Henderson]

On 05/18/12 03:48, Chung-Lin Tang wrote:
> @@ -2401,6 +2401,7 @@ scan_trace (dw_trace_info *trace)
>  	{
>  	  /* Propagate across fallthru edges.  */
>  	  dwarf2out_flush_queued_reg_saves ();
> +	  def_cfa_1 (&this_cfa);
>  	  maybe_record_trace_start (insn, NULL);
>  	  break;
>  	}
> @@ -2455,6 +2456,18 @@ scan_trace (dw_trace_info *trace)
>  		  cur_cfa = &this_cfa;
>  		  continue;
>  		}
> +	      else
> +		{
> +		  /* If ELT is a annulled branch-taken instruction (i.e. executed
> +		     only when branch is not taken), the args_size and CFA should
> +		     not change through the jump.  */
> +		  create_trace_edges (control);
> +
> +		  /* Update and continue with the trace.  */
> +		  add_cfi_insn = insn;
> +		  scan_insn_after (elt);
> +		  continue;
> +		}

I think the def_cfa_1 is misplaced.  It should be immediately before
that last continue.  That mirrors the sort of flow you get via the
other paths through the loop.



r~

Re: [PATCH 1/2] SH epilogue unwind, dwarf2 pass changes

[Chung-Lin Tang]

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

On 12/5/23 1:46 AM, Richard Henderson wrote:
> On 05/18/12 03:48, Chung-Lin Tang wrote:
>> @@ -2401,6 +2401,7 @@ scan_trace (dw_trace_info *trace)
>>  	{
>>  	  /* Propagate across fallthru edges.  */
>>  	  dwarf2out_flush_queued_reg_saves ();
>> +	  def_cfa_1 (&this_cfa);
>>  	  maybe_record_trace_start (insn, NULL);
>>  	  break;
>>  	}
>> @@ -2455,6 +2456,18 @@ scan_trace (dw_trace_info *trace)
>>  		  cur_cfa = &this_cfa;
>>  		  continue;
>>  		}
>> +	      else
>> +		{
>> +		  /* If ELT is a annulled branch-taken instruction (i.e. executed
>> +		     only when branch is not taken), the args_size and CFA should
>> +		     not change through the jump.  */
>> +		  create_trace_edges (control);
>> +
>> +		  /* Update and continue with the trace.  */
>> +		  add_cfi_insn = insn;
>> +		  scan_insn_after (elt);
>> +		  continue;
>> +		}
> 
> I think the def_cfa_1 is misplaced.  It should be immediately before
> that last continue.  That mirrors the sort of flow you get via the
> other paths through the loop.

Or possibly moved before the dwarf2out_flush_queued_reg_saves () call in
the patch? (in the save_point_p () break case)  Note I'm only saying
this based on overall ordering of those two routine calls in the loop.

Attached is a testcase (adapted from libgomp) that, with the SH epilogue
unwind patch applied alone, produces the ICE I'm seeing (-O2
-funwind-tables).

This dwarf2 patch, with any of the three new def_cfa_1() call sites
seems to solve it, though you might want to comment on which call site
seems "correct"

Thanks,
Chung-Lin

[-- Attachment #2: g.c --]
[-- Type: text/plain, Size: 516 bytes --]

typedef unsigned long long gomp_ull;
int
foo (gomp_ull *pstart, gomp_ull *pend, int mode,
     gomp_ull next_ull, gomp_ull end_ull, gomp_ull chunk_size_ull)
{
  gomp_ull start, end, chunk, left;
  start = next_ull;
  if (start == end_ull)
    return 0;

  chunk = chunk_size_ull;
  left = end_ull - start;
  if (mode & 2)
    {
      if (chunk < left)
        chunk = left;
    }
  else
    {
      if (chunk > left)
        chunk = left;
    }
  end = start + chunk;
  *pstart = start;
  *pend = end;
  return 1;
}

Re: [PATCH 1/2] SH epilogue unwind, dwarf2 pass changes

[Chung-Lin Tang]

Ping?

On 2012/6/1 06:24 PM, Chung-Lin Tang wrote:
> On 12/5/23 1:46 AM, Richard Henderson wrote:
>> On 05/18/12 03:48, Chung-Lin Tang wrote:
>>> @@ -2401,6 +2401,7 @@ scan_trace (dw_trace_info *trace)
>>>  	{
>>>  	  /* Propagate across fallthru edges.  */
>>>  	  dwarf2out_flush_queued_reg_saves ();
>>> +	  def_cfa_1 (&this_cfa);
>>>  	  maybe_record_trace_start (insn, NULL);
>>>  	  break;
>>>  	}
>>> @@ -2455,6 +2456,18 @@ scan_trace (dw_trace_info *trace)
>>>  		  cur_cfa = &this_cfa;
>>>  		  continue;
>>>  		}
>>> +	      else
>>> +		{
>>> +		  /* If ELT is a annulled branch-taken instruction (i.e. executed
>>> +		     only when branch is not taken), the args_size and CFA should
>>> +		     not change through the jump.  */
>>> +		  create_trace_edges (control);
>>> +
>>> +		  /* Update and continue with the trace.  */
>>> +		  add_cfi_insn = insn;
>>> +		  scan_insn_after (elt);
>>> +		  continue;
>>> +		}
>>
>> I think the def_cfa_1 is misplaced.  It should be immediately before
>> that last continue.  That mirrors the sort of flow you get via the
>> other paths through the loop.
> 
> Or possibly moved before the dwarf2out_flush_queued_reg_saves () call in
> the patch? (in the save_point_p () break case)  Note I'm only saying
> this based on overall ordering of those two routine calls in the loop.
> 
> Attached is a testcase (adapted from libgomp) that, with the SH epilogue
> unwind patch applied alone, produces the ICE I'm seeing (-O2
> -funwind-tables).
> 
> This dwarf2 patch, with any of the three new def_cfa_1() call sites
> seems to solve it, though you might want to comment on which call site
> seems "correct"
> 
> Thanks,
> Chung-Lin

Re: [PATCH 1/2] SH epilogue unwind, dwarf2 pass changes

[Richard Henderson]

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

On 2012-06-11 01:34, Chung-Lin Tang wrote:
> Ping?
> 
> On 2012/6/1 06:24 PM, Chung-Lin Tang wrote:
>> On 12/5/23 1:46 AM, Richard Henderson wrote:
>>> On 05/18/12 03:48, Chung-Lin Tang wrote:
>>>> @@ -2401,6 +2401,7 @@ scan_trace (dw_trace_info *trace)
>>>>  	{
>>>>  	  /* Propagate across fallthru edges.  */
>>>>  	  dwarf2out_flush_queued_reg_saves ();
>>>> +	  def_cfa_1 (&this_cfa);
>>>>  	  maybe_record_trace_start (insn, NULL);
>>>>  	  break;
>>>>  	}
>>>> @@ -2455,6 +2456,18 @@ scan_trace (dw_trace_info *trace)
>>>>  		  cur_cfa = &this_cfa;
>>>>  		  continue;
>>>>  		}
>>>> +	      else
>>>> +		{
>>>> +		  /* If ELT is a annulled branch-taken instruction (i.e. executed
>>>> +		     only when branch is not taken), the args_size and CFA should
>>>> +		     not change through the jump.  */
>>>> +		  create_trace_edges (control);
>>>> +
>>>> +		  /* Update and continue with the trace.  */
>>>> +		  add_cfi_insn = insn;
>>>> +		  scan_insn_after (elt);
>>>> +		  continue;
>>>> +		}
>>>
>>> I think the def_cfa_1 is misplaced.  It should be immediately before
>>> that last continue.  That mirrors the sort of flow you get via the
>>> other paths through the loop.
>>
>> Or possibly moved before the dwarf2out_flush_queued_reg_saves () call in
>> the patch? (in the save_point_p () break case)  Note I'm only saying
>> this based on overall ordering of those two routine calls in the loop.
>>
>> Attached is a testcase (adapted from libgomp) that, with the SH epilogue
>> unwind patch applied alone, produces the ICE I'm seeing (-O2
>> -funwind-tables).
>>
>> This dwarf2 patch, with any of the three new def_cfa_1() call sites
>> seems to solve it, though you might want to comment on which call site
>> seems "correct"

I've committed the following, after a --enable-languages=c,c++ --target=sh-elf
build and check-gcc with your patch 2/2 applied.  I eyeballed the resulting
debug info and it looked correct.

Given the test case was extracted from libgomp I didn't commit that, trusting
that the real testing will continue to happen during a build.


r~

[-- Attachment #2: z --]
[-- Type: text/plain, Size: 1736 bytes --]

	* dwarf2cfi.c (scan_trace): Handle annulled branch-taken delay slots.



diff --git a/gcc/dwarf2cfi.c b/gcc/dwarf2cfi.c
index bf2d802f..3edb6e1 100644
--- a/gcc/dwarf2cfi.c
+++ b/gcc/dwarf2cfi.c
@@ -2429,18 +2429,20 @@ scan_trace (dw_trace_info *trace)
 
 	      elt = XVECEXP (pat, 0, 1);
 
-	      /* If ELT is an instruction from target of an annulled branch,
-		 the effects are for the target only and so the args_size
-		 and CFA along the current path shouldn't change.  */
 	      if (INSN_FROM_TARGET_P (elt))
 		{
 		  HOST_WIDE_INT restore_args_size;
 		  cfi_vec save_row_reg_save;
 
+		  /* If ELT is an instruction from target of an annulled
+		     branch, the effects are for the target only and so
+		     the args_size and CFA along the current path
+		     shouldn't change.  */
 		  add_cfi_insn = NULL;
 		  restore_args_size = cur_trace->end_true_args_size;
 		  cur_cfa = &cur_row->cfa;
-		  save_row_reg_save = VEC_copy (dw_cfi_ref, gc, cur_row->reg_save);
+		  save_row_reg_save
+		    = VEC_copy (dw_cfi_ref, gc, cur_row->reg_save);
 
 		  scan_insn_after (elt);
 
@@ -2453,8 +2455,20 @@ scan_trace (dw_trace_info *trace)
 		  cur_row->cfa = this_cfa;
 		  cur_row->reg_save = save_row_reg_save;
 		  cur_cfa = &this_cfa;
-		  continue;
 		}
+	      else
+		{
+		  /* If ELT is a annulled branch-taken instruction (i.e.
+		     executed only when branch is not taken), the args_size
+		     and CFA should not change through the jump.  */
+		  create_trace_edges (control);
+
+		  /* Update and continue with the trace.  */
+		  add_cfi_insn = insn;
+		  scan_insn_after (elt);
+		  def_cfa_1 (&this_cfa);
+		}
+	      continue;
 	    }
 
 	  /* The insns in the delay slot should all be considered to happen