[Patch] Fix bug for frame instructions in annulled delay slots

[Patch] Fix bug for frame instructions in annulled delay slots

[Steve Ellcey]

While doing more testing of the MIPS frame header optimization I checked
in earlier, I found I could not build the elf toolchain when I turned the
optimization on by default.  This is because GCC was putting a frame related
instruction (the save of the return address) into an annulled delay slot.
This only happens when the function prologue is moved after a branch by the
shrink wrap optimization and it caused create_cfi_notes to abort because the
code that generates the CFI information cannot handle stack related annulled
instructions.

This never occurred when I did my testing with the linux toolchain because
I did not use -Os (which caused the use of instructions with annulled delay
slots) and because -O2 does more inlining than -Os which meant there were
fewer places where the return address could be saved in the callers frame
header without the callee having to allocate its own stack space.

The fix for this is not in mips specific code but in reorg.c where we
restrict GCC from putting any frame related instructions into annulled
delay slots.  There was already code in reorg.c to prevent frame related
instructions from going into normal delay slots but that code did not seem
to cover all the unnulled delay slot situations.  This patch extends the
code to not put frame related instructions into annulled delay slots.

Tested on MIPS with linux and elf toolchain builds and testsuite runs.

OK to checkin?

Steve Ellcey
sellcey@imgtec.com


2015-12-07  Steve Ellcey  <sellcey@imgtec.com>

	* reorg.c (optimize_skip): Do not put frame related instructions
	in annulled delay slots.
	(steal_delay_list_from_target): Ditto.
	(fill_slots_from_thread): Ditto.


diff --git a/gcc/reorg.c b/gcc/reorg.c
index cc68d6b..bab5f49 100644
--- a/gcc/reorg.c
+++ b/gcc/reorg.c
@@ -739,6 +739,7 @@ optimize_skip (rtx_jump_insn *insn, vec<rtx_insn *> *delay_list)
       || recog_memoized (trial) < 0
       || (! eligible_for_annul_false (insn, 0, trial, flags)
 	  && ! eligible_for_annul_true (insn, 0, trial, flags))
+      || RTX_FRAME_RELATED_P (trial)
       || can_throw_internal (trial))
     return;
 
@@ -1126,7 +1127,13 @@ steal_delay_list_from_target (rtx_insn *insn, rtx condition, rtx_sequence *seq,
 					      trial, flags)))
 	{
 	  if (must_annul)
-	    used_annul = 1;
+	    {
+	      /* Frame related instructions cannot go into annulled delay
+		 slots, it messes up the dwarf info.  */
+	      if (RTX_FRAME_RELATED_P (trial))
+		return;
+	      used_annul = 1;
+	    }
 	  rtx_insn *temp = copy_delay_slot_insn (trial);
 	  INSN_FROM_TARGET_P (temp) = 1;
 	  add_to_delay_list (temp, &new_delay_list);
@@ -2464,9 +2471,9 @@ fill_slots_from_thread (rtx_jump_insn *insn, rtx condition,
 	      if (eligible_for_delay (insn, *pslots_filled, trial, flags))
 		goto winner;
 	    }
-	  else if (0
-		   || (ANNUL_IFTRUE_SLOTS && ! thread_if_true)
-		   || (ANNUL_IFFALSE_SLOTS && thread_if_true))
+	  else if (!RTX_FRAME_RELATED_P (trial) \
+		   && ((ANNUL_IFTRUE_SLOTS && ! thread_if_true)
+		        || (ANNUL_IFFALSE_SLOTS && thread_if_true)))
 	    {
 	      old_trial = trial;
 	      trial = try_split (pat, trial, 0);



2015-12-07  Steve Ellcey  <sellcey@imgtec.com>

	* gcc.target/mips/wrap-delay.c: New test.


diff --git a/gcc/testsuite/gcc.target/mips/wrap-delay.c b/gcc/testsuite/gcc.target/mips/wrap-delay.c
index e69de29..1332a68 100644
--- a/gcc/testsuite/gcc.target/mips/wrap-delay.c
+++ b/gcc/testsuite/gcc.target/mips/wrap-delay.c
@@ -0,0 +1,42 @@
+/* { dg-do compile } */
+/* { dg-options "-g -mframe-header-opt -mbranch-likely" } */
+
+/* GCC was generating an ICE with the above options and -Os because
+   it was putting the save of $31 in two annulled delay slots but
+   there was only one restore since only one of the two saves could be
+   executed.  This was correct code but it confused dwarf2cfi and
+   caused it to ICE when using the -g option.  */
+
+
+enum demangle_component_type
+{
+	DEMANGLE_COMPONENT_TRINARY_ARG2,
+};
+struct demangle_component
+{
+  enum demangle_component_type type;
+};
+struct d_info
+{
+  int next_comp;
+  int num_comps;
+};
+struct demangle_component * d_make_empty (struct d_info *di)
+{
+  if (di->next_comp >= di->num_comps) return ((void *)0);
+  ++di->next_comp;
+}
+struct demangle_component *d_make_comp (
+	struct d_info *di,
+	enum demangle_component_type type,
+	struct demangle_component *left)
+{
+  struct demangle_component *p;
+  switch (type)
+    {
+    case DEMANGLE_COMPONENT_TRINARY_ARG2:
+      if (left == ((void *)0)) return ((void *)0);
+    }
+  p = d_make_empty (di);
+  p->type = type;
+}

Re: [Patch] Fix bug for frame instructions in annulled delay slots

[Bernd Schmidt]

On 12/07/2015 07:54 PM, Steve Ellcey  wrote:
>   	  if (must_annul)
> -	    used_annul = 1;
> +	    {
> +	      /* Frame related instructions cannot go into annulled delay
> +		 slots, it messes up the dwarf info.  */
> +	      if (RTX_FRAME_RELATED_P (trial))
> +		return;

Don't you need to use break rather than return?

> +	  else if (!RTX_FRAME_RELATED_P (trial) \

Stray backslash.

Other than that I think this is OK. There are some preexisting tests for 
frame related insns already in this code.


Bernd

Re: [Patch] Fix bug for frame instructions in annulled delay slots

[Jeff Law]

On 12/07/2015 12:28 PM, Bernd Schmidt wrote:
> On 12/07/2015 07:54 PM, Steve Ellcey  wrote:
>>         if (must_annul)
>> -        used_annul = 1;
>> +        {
>> +          /* Frame related instructions cannot go into annulled delay
>> +         slots, it messes up the dwarf info.  */
>> +          if (RTX_FRAME_RELATED_P (trial))
>> +        return;
>
> Don't you need to use break rather than return?
>
>> +      else if (!RTX_FRAME_RELATED_P (trial) \
>
> Stray backslash.
>
> Other than that I think this is OK. There are some preexisting tests for
> frame related insns already in this code.
Also note there's probably port cleanup that could happen once this goes 
in.  IIRC the PA port (for example) explicitly disallows frame related 
insns from many (most, all?) delay slots.  Other targets may be doing 
something similar.

jeff

Re: [Patch] Fix bug for frame instructions in annulled delay slots

[Steve Ellcey]

On Mon, 2015-12-07 at 12:30 -0700, Jeff Law wrote:
> On 12/07/2015 12:28 PM, Bernd Schmidt wrote:
> > On 12/07/2015 07:54 PM, Steve Ellcey  wrote:
> >>         if (must_annul)
> >> -        used_annul = 1;
> >> +        {
> >> +          /* Frame related instructions cannot go into annulled delay
> >> +         slots, it messes up the dwarf info.  */
> >> +          if (RTX_FRAME_RELATED_P (trial))
> >> +        return;
> >
> > Don't you need to use break rather than return?
> >
> >> +      else if (!RTX_FRAME_RELATED_P (trial) \
> >
> > Stray backslash
> > Other than that I think this is OK. There are some preexisting tests for
> > frame related insns already in this code.
> Also note there's probably port cleanup that could happen once this goes 
> in.  IIRC the PA port (for example) explicitly disallows frame related 
> insns from many (most, all?) delay slots.  Other targets may be doing 
> something similar.
> 
> jeff

If I had remembered/seen that code in pa.md earlier I might have fixed
MIPS the same way.  Oh well, I guess changing reorg.c is the better way
to do it since it can be shared now.

It looks like steal_delay_list_from_fallthrough is missing a lot of
FRAME_RELATED checks (and other things) that
steal_delay_list_from_target has in it (not just my changes).  I think
that means most of the work/fixing has been on machines like MIPS where
the code in the delay slot is either always executed or only
executed when the branch is taken (vs. untaken).  I think
steal_delay_list_from_fallthrough is only going to be used when the
delay slot is executed on a branch-not-taken.  Are there any machines
like that?

Steve Ellcey
sellcey@imgtec.com

Re: [Patch] Fix bug for frame instructions in annulled delay slots

[Jeff Law]

On 12/07/2015 02:18 PM, Steve Ellcey wrote:
> On Mon, 2015-12-07 at 12:30 -0700, Jeff Law wrote:
>> On 12/07/2015 12:28 PM, Bernd Schmidt wrote:
>>> On 12/07/2015 07:54 PM, Steve Ellcey  wrote:
>>>>          if (must_annul)
>>>> -        used_annul = 1;
>>>> +        {
>>>> +          /* Frame related instructions cannot go into annulled delay
>>>> +         slots, it messes up the dwarf info.  */
>>>> +          if (RTX_FRAME_RELATED_P (trial))
>>>> +        return;
>>>
>>> Don't you need to use break rather than return?
>>>
>>>> +      else if (!RTX_FRAME_RELATED_P (trial) \
>>>
>>> Stray backslash
>>> Other than that I think this is OK. There are some preexisting tests for
>>> frame related insns already in this code.
>> Also note there's probably port cleanup that could happen once this goes
>> in.  IIRC the PA port (for example) explicitly disallows frame related
>> insns from many (most, all?) delay slots.  Other targets may be doing
>> something similar.
>>
>> jeff
>
> If I had remembered/seen that code in pa.md earlier I might have fixed
> MIPS the same way.  Oh well, I guess changing reorg.c is the better way
> to do it since it can be shared now.
Agreed.

> executed when the branch is taken (vs. untaken).  I think
> steal_delay_list_from_fallthrough is only going to be used when the
> delay slot is executed on a branch-not-taken.  Are there any machines
> like that?
?  I don't think it's restricted to cases where the delay slot is 
executed only on a branch-not-taken.

Jeff

Re: [Patch] Fix bug for frame instructions in annulled delay slots

[Steve Ellcey]

On Mon, 2015-12-07 at 20:28 +0100, Bernd Schmidt wrote:
> On 12/07/2015 07:54 PM, Steve Ellcey  wrote:
> >   	  if (must_annul)
> > -	    used_annul = 1;
> > +	    {
> > +	      /* Frame related instructions cannot go into annulled delay
> > +		 slots, it messes up the dwarf info.  */
> > +	      if (RTX_FRAME_RELATED_P (trial))
> > +		return;
> 
> Don't you need to use break rather than return?

I am not sure about this.  There is an earlier if statement in the loop
that does a 'return' instead of a break (or continue) and there is a 
return in the 'else' part of the if that sets must_annul.  Both of these
are inside the loop that looks at all the instructions in the sequence
'seq'.  I think the code is looking at all the instructions in the
sequence and if any of them fail one of the tests in the loop (in this
case require annulling) then we can't handle any of the instructions in
the sequence and so we return immediately without putting any of the
instructions from 'seq' in the delay slot.  I believe a frame related
instruction in an annulled branch needs to be handled that way too.

> 
> > +	  else if (!RTX_FRAME_RELATED_P (trial) \
> 
> Stray backslash.

That is easily fixed.

> Other than that I think this is OK. There are some preexisting tests for 
> frame related insns already in this code.
> 
> 
> Bernd
> 

Re: [Patch] Fix bug for frame instructions in annulled delay slots

[Bernd Schmidt]

On 12/07/2015 08:43 PM, Steve Ellcey wrote:
> I am not sure about this.  There is an earlier if statement in the loop
> that does a 'return' instead of a break (or continue) and there is a
> return in the 'else' part of the if that sets must_annul.  Both of these
> are inside the loop that looks at all the instructions in the sequence
> 'seq'.  I think the code is looking at all the instructions in the
> sequence and if any of them fail one of the tests in the loop (in this
> case require annulling) then we can't handle any of the instructions in
> the sequence and so we return immediately without putting any of the
> instructions from 'seq' in the delay slot.  I believe a frame related
> instruction in an annulled branch needs to be handled that way too.

Ah, I think I was looking at the other function that has the same 
must_annul test (steal_delay_list_from_fallthrough). The patch is ok 
without the backslash. Maybe the other function ought to be changed as 
well though?


Bernd

Re: [Patch] Fix bug for frame instructions in annulled delay slots

[Steve Ellcey]

On Mon, 2015-12-07 at 20:59 +0100, Bernd Schmidt wrote:
> On 12/07/2015 08:43 PM, Steve Ellcey wrote:
> > I am not sure about this.  There is an earlier if statement in the loop
> > that does a 'return' instead of a break (or continue) and there is a
> > return in the 'else' part of the if that sets must_annul.  Both of these
> > are inside the loop that looks at all the instructions in the sequence
> > 'seq'.  I think the code is looking at all the instructions in the
> > sequence and if any of them fail one of the tests in the loop (in this
> > case require annulling) then we can't handle any of the instructions in
> > the sequence and so we return immediately without putting any of the
> > instructions from 'seq' in the delay slot.  I believe a frame related
> > instruction in an annulled branch needs to be handled that way too.
> 
> Ah, I think I was looking at the other function that has the same 
> must_annul test (steal_delay_list_from_fallthrough). The patch is ok 
> without the backslash. Maybe the other function ought to be changed as 
> well though?
> 
> 
> Bernd

That would seem reasonable, though I don't have a test case for where
the change to that routine would actually make a difference in the
compilation.  I'll create a patch and test it to make sure it doesn't
cause any problems and then submit it as a follow-up to this change.

Steve Ellcey
sellcey@imgtec.com