diff mbox

[committed] PR 51931: force non-MIPS16ness for long-branch tests (NOW RFA: MIPS16 Long Branch Patch)

Message ID FD3DCEAC5B03E9408544A1E416F1124211F497BE@NA-MBX-04.mgc.mentorg.com
State New
Headers show

Commit Message

Moore, Catherine Jan. 30, 2012, 9:05 p.m. UTC
> -----Original Message-----
> From: Chung-Lin Tang [mailto:cltang@codesourcery.com]
> Sent: Monday, January 30, 2012 4:36 AM
> To: gcc-patches@gcc.gnu.org; rdsandiford@googlemail.com
> Cc: Moore, Catherine
> Subject: Re: [committed] PR 51931: force non-MIPS16ness for long-branch tests
> 
> On 2012/1/22 06:33 PM, Richard Sandiford wrote:
> > The MIPS16 port has never handled long branches properly; see PR 51931
> > for the details.  It isn't easy to xfail MIPS16-specific problems at
> > the dejagnu level because of -mflip-mips16, so the patch below forces
> > a nomips16 attribute instead.
> >
> > Tested on mips64-linux-gnu and applied.
> >
> > Richard
> 
> CCing Catherine, I think we have a fix for this?
> 

I do have a patch.   It's a heuristic and will not work in all instances, but it does allow many additional programs to successfully compile.  For example, this scheme allowed me to build glibc in MIPS16-mode for a MIPS-Linux toolchain.  

The patch causes reorg to examine mips16 branches.  For branches that are out-of-range, reorg will look for branches to the same target.  If that branch is in range, the destination of the original branch becomes the new branch.  If branches to the same target do not exist, then reorg will search for barriers that are in range and insert label+ branch at the barrier.

Of the test cases mentioned in the bug report, gcc.c-torture/compile/20001226-1.c still fails due to a lack of barriers in the instruction stream.  g++.dg/opt/longbranch1.C will pass.

I've set off a test run with my patch applied against mainline.   In the meantime, here's the patch.  Richard, what do you think?

Catherine

2012-01-30  Catherine Moore  <clm@codesourcery.com>

        gcc/
        * config/mips/mips.c (WALK_INSNS): New macro.
        (OK_FOR_MIPS16_BRANCH_OFFSET): New macro.
        (mips16_count_matching_branches): New function.
        (walk_insns_forward): New function.
        (walk_insns_reverse): New function.
        (mips16_update_branch_info): New function.
        (mips16_zero_branch_info): New function.
        (mips16_check_branches): New function.
        (mips_reorg_process_instructions): Call mips16_check_branches.
        * config/mips.h (mips16_branch_info_t): New typedef.

Comments

Richard Sandiford Feb. 6, 2012, 7:51 p.m. UTC | #1
Thanks for the patch.

"Moore, Catherine" <Catherine_Moore@mentor.com> writes:
>> -----Original Message-----
>> From: Chung-Lin Tang [mailto:cltang@codesourcery.com]
>> Sent: Monday, January 30, 2012 4:36 AM
>> To: gcc-patches@gcc.gnu.org; rdsandiford@googlemail.com
>> Cc: Moore, Catherine
>> Subject: Re: [committed] PR 51931: force non-MIPS16ness for long-branch tests
>> 
>> On 2012/1/22 06:33 PM, Richard Sandiford wrote:
>> > The MIPS16 port has never handled long branches properly; see PR 51931
>> > for the details.  It isn't easy to xfail MIPS16-specific problems at
>> > the dejagnu level because of -mflip-mips16, so the patch below forces
>> > a nomips16 attribute instead.
>> >
>> > Tested on mips64-linux-gnu and applied.
>> >
>> > Richard
>> 
>> CCing Catherine, I think we have a fix for this?
>> 
>
> I do have a patch.  It's a heuristic and will not work in all
> instances, but it does allow many additional programs to successfully
> compile.  For example, this scheme allowed me to build glibc in
> MIPS16-mode for a MIPS-Linux toolchain.
>
> The patch causes reorg to examine mips16 branches.  For branches that
> are out-of-range, reorg will look for branches to the same target.  If
> that branch is in range, the destination of the original branch
> becomes the new branch.  If branches to the same target do not exist,
> then reorg will search for barriers that are in range and insert
> label+ branch at the barrier.
>
> Of the test cases mentioned in the bug report,
> gcc.c-torture/compile/20001226-1.c still fails due to a lack of
> barriers in the instruction stream.  g++.dg/opt/longbranch1.C will
> pass.
>
> I've set off a test run with my patch applied against mainline.  In
> the meantime, here's the patch.  Richard, what do you think?

Yeah, it's difficult.  On the one hand, this is probably more efficient
(both in terms of code size and speed) than a MIPS16 equivalent of the
non-MIPS16 fallback, which uses a label load followed by an indirect jump.
On the other hand, it can suffer from degenerate cases where we need so
many new branches that even the trampolines become out of range.
(Maybe that's what's happening in the 20001226-1.c case.)

Since this isn't a regression, the patch would need to wait for 4.8 anyway.
I'll have a think about it before then (or at least try to remember to...)

Thanks,
Richard
Moore, Catherine July 18, 2012, 4:55 p.m. UTC | #2
Hi Richard,

Now that we are in the window for 4.8, I'd like to discuss the possibility of applying this patch.  Have you had a chance to think about it?

Thanks,
Catherine

> -----Original Message-----
> From: Richard Sandiford [mailto:rdsandiford@googlemail.com]
> Sent: Monday, February 06, 2012 2:51 PM
> To: Moore, Catherine
> Cc: Tang, Chung-Lin; gcc-patches@gcc.gnu.org
> Subject: Re: [committed] PR 51931: force non-MIPS16ness for long-branch tests
> (NOW RFA: MIPS16 Long Branch Patch)
> 
> Thanks for the patch.
> 
> "Moore, Catherine" <Catherine_Moore@mentor.com> writes:
> >> -----Original Message-----
> >> From: Chung-Lin Tang [mailto:cltang@codesourcery.com]
> >> Sent: Monday, January 30, 2012 4:36 AM
> >> To: gcc-patches@gcc.gnu.org; rdsandiford@googlemail.com
> >> Cc: Moore, Catherine
> >> Subject: Re: [committed] PR 51931: force non-MIPS16ness for
> >> long-branch tests
> >>
> >> On 2012/1/22 06:33 PM, Richard Sandiford wrote:
> >> > The MIPS16 port has never handled long branches properly; see PR
> >> > 51931 for the details.  It isn't easy to xfail MIPS16-specific
> >> > problems at the dejagnu level because of -mflip-mips16, so the
> >> > patch below forces a nomips16 attribute instead.
> >> >
> >> > Tested on mips64-linux-gnu and applied.
> >> >
> >> > Richard
> >>
> >> CCing Catherine, I think we have a fix for this?
> >>
> >
> > I do have a patch.  It's a heuristic and will not work in all
> > instances, but it does allow many additional programs to successfully
> > compile.  For example, this scheme allowed me to build glibc in
> > MIPS16-mode for a MIPS-Linux toolchain.
> >
> > The patch causes reorg to examine mips16 branches.  For branches that
> > are out-of-range, reorg will look for branches to the same target.  If
> > that branch is in range, the destination of the original branch
> > becomes the new branch.  If branches to the same target do not exist,
> > then reorg will search for barriers that are in range and insert
> > label+ branch at the barrier.
> >
> > Of the test cases mentioned in the bug report,
> > gcc.c-torture/compile/20001226-1.c still fails due to a lack of
> > barriers in the instruction stream.  g++.dg/opt/longbranch1.C will
> > pass.
> >
> > I've set off a test run with my patch applied against mainline.  In
> > the meantime, here's the patch.  Richard, what do you think?
> 
> Yeah, it's difficult.  On the one hand, this is probably more efficient (both in
> terms of code size and speed) than a MIPS16 equivalent of the
> non-MIPS16 fallback, which uses a label load followed by an indirect jump.
> On the other hand, it can suffer from degenerate cases where we need so many
> new branches that even the trampolines become out of range.
> (Maybe that's what's happening in the 20001226-1.c case.)
> 
> Since this isn't a regression, the patch would need to wait for 4.8 anyway.
> I'll have a think about it before then (or at least try to remember to...)
> 
> Thanks,
> Richard
diff mbox

Patch

Index: mips.c
===================================================================
--- mips.c	(revision 183732)
+++ mips.c	(working copy)
@@ -123,6 +123,12 @@ 
        (SUBINSN) != NEXT_INSN (SEQ_END (INSN));				\
        (SUBINSN) = NEXT_INSN (SUBINSN))
 
+/* Walk the instruction chain either backwards or forwards.  */
+#define WALK_INSNS(START_INSN, INSN, WALK_FUNCTION) 			\
+  for ((INSN) = (WALK_FUNCTION) (START_INSN);				\
+       (INSN) != 0;							\
+       (INSN) = (WALK_FUNCTION) (INSN))
+
 /* True if bit BIT is set in VALUE.  */
 #define BITSET_P(VALUE, BIT) (((VALUE) & (1 << (BIT))) != 0)
 
@@ -14938,7 +14944,214 @@ 
     }
   return INTVAL (offset) <= entry->offset;
 }
+#define OK_FOR_MIPS16_BRANCH(OFFSET) ((OFFSET) >= -65000 && (OFFSET) <= 65000)
 
+static int
+mips16_count_matching_branches (rtx jump_insn)
+{
+  rtx insn;
+  int label_uid, jump_uid;
+  int matches = 0;
+  
+  jump_uid = INSN_UID (jump_insn);
+  label_uid = INSN_UID (JUMP_LABEL (jump_insn));
+
+  /* Count the number of branches to the same label.  */
+  for (insn = get_insns (); insn != 0; insn = NEXT_INSN (insn))
+    {
+      if (simplejump_p (insn))
+	{
+	  int insn_uid = INSN_UID (JUMP_LABEL (insn));
+	  if (insn_uid != jump_uid && label_uid == insn_uid)
+	    matches++;
+	}
+    }
+
+  return matches;
+}
+
+static rtx
+walk_insns_forward (rtx insn)
+{
+  return NEXT_INSN (insn);
+}
+
+static rtx
+walk_insns_reverse (rtx insn)
+{
+  return PREV_INSN (insn);
+}
+
+static void
+mips16_update_branch_info (mips16_branch_info_t *previous,
+			   mips16_branch_info_t *current,
+			   int insn_uid)
+{
+  /* Move current to previous.  */
+  previous->uid = current->uid;
+  previous->address = current->address;
+  previous->distance_to_dest = current->distance_to_dest;
+  previous->distance_from_src = current->distance_from_src;
+
+  /* Update current.  */
+  current->uid = insn_uid;
+  current->address = INSN_ADDRESSES (insn_uid);
+}
+
+static void
+mips16_zero_branch_info (mips16_branch_info_t *branch_info)
+{
+  branch_info->uid = 0;
+  branch_info->address = 0;
+  branch_info->distance_to_dest = 0;
+  branch_info->distance_from_src = 0;
+}
+
+static void
+mips16_check_branches (rtx jump_insn)
+{
+  int label1_uid, label2_uid, jump_uid, insn_uid;
+  int offset;
+  rtx label1;
+  rtx insn, new_label, new_label_insn, new_jump, new_jump_label;
+
+  rtx (*walk_function) (rtx);
+
+  mips16_branch_info_t first_branch, current, previous;
+  
+  mips16_zero_branch_info (&first_branch);
+  mips16_zero_branch_info (&current);
+  mips16_zero_branch_info (&previous);
+
+  jump_uid = INSN_UID (jump_insn);
+  first_branch.uid = jump_uid;
+  first_branch.address = INSN_ADDRESSES (jump_uid);
+
+  /* Don't reprocess branches that were introduced by mips16_check_branches.
+     The branches are known to be within range.  We'll update INSN_ADDRESSES
+     for all of the new branches when we finish here.  */
+  if (first_branch.address == -1)
+    return;
+
+  label1 = JUMP_LABEL (jump_insn);
+  label1_uid = INSN_UID (label1);
+
+  offset = INSN_ADDRESSES (label1_uid) - INSN_ADDRESSES (jump_uid);
+  first_branch.distance_to_dest = offset;
+
+  /* The branch offset is in range, nothing to do.  */
+  if (OK_FOR_MIPS16_BRANCH (offset))
+    return;
+
+  if (offset > 0)
+    walk_function = &walk_insns_forward;
+  else
+    walk_function = &walk_insns_reverse;
+
+  /* The branch offset is too large.  First figure out if there are
+     other branches to the same destination.  If other branches to 
+     the same destination are available, we'll branch there instead.  */
+
+  if (mips16_count_matching_branches (jump_insn))
+    {
+      /* If branches to the same label are available and in range,
+	 then use those first.  */
+      WALK_INSNS (jump_insn, insn, walk_function)
+	{
+	  if (JUMP_P (insn))
+	    {
+	      insn_uid = INSN_UID (insn);
+	      label2_uid = INSN_UID (JUMP_LABEL (insn));
+
+	      if (label2_uid != label1_uid)
+		continue;
+
+	      mips16_update_branch_info (&previous, &current, insn_uid);
+	      offset = INSN_ADDRESSES (insn_uid) - INSN_ADDRESSES (jump_uid);
+	      current.distance_from_src = offset;
+	      if (!OK_FOR_MIPS16_BRANCH (offset))
+		{
+		  /* If the previous candidate was in range, use it.  */
+		  if (previous.address != 0
+		      && OK_FOR_MIPS16_BRANCH (previous.distance_from_src))
+		    {
+		      new_label = gen_label_rtx ();
+		      emit_label_before (new_label, insn);
+		      redirect_jump (jump_insn, new_label, 0);
+		      return;
+		    }
+		  continue;
+		}
+
+	      offset = INSN_ADDRESSES (label2_uid) - INSN_ADDRESSES (insn_uid);
+	      current.distance_to_dest = offset;
+	      if (!OK_FOR_MIPS16_BRANCH (offset))
+		continue; 
+        
+	      /* The offset is in range.  The new destination will
+		 be this branch.  */
+	      new_label = gen_label_rtx ();
+	      emit_label_before (new_label, insn);
+	      redirect_jump (jump_insn, new_label, 0);
+	      return;
+	    }
+	}
+    }
+
+  /* Reset current and previous branch information.  */
+  mips16_zero_branch_info (&current);
+  mips16_zero_branch_info (&previous);
+
+  /* Search for a barrier instruction/insert a label and a jump.  */
+  WALK_INSNS (jump_insn, insn, walk_function)
+    {
+      int barrier_off, label_off;
+
+      if (GET_CODE (insn) == BARRIER)
+	{
+	  insn_uid = INSN_UID (insn);
+	  mips16_update_branch_info (&previous, &current, insn_uid);
+
+	  barrier_off = INSN_ADDRESSES (insn_uid) - INSN_ADDRESSES (jump_uid);
+	  label_off = INSN_ADDRESSES (label1_uid) - INSN_ADDRESSES (insn_uid);
+	  current.distance_from_src = barrier_off;
+	  current.distance_to_dest = label_off;
+
+	  if (!OK_FOR_MIPS16_BRANCH (barrier_off))
+	    {
+	      /* If the previous candidate was in range, use that instead.  */
+	      if (previous.address != 0
+		  && OK_FOR_MIPS16_BRANCH (previous.distance_from_src))
+		{
+		  new_label = gen_label_rtx ();
+		  new_label_insn = emit_label_before (new_label, insn);
+		  redirect_jump (jump_insn, new_label, 0);
+		  new_jump_label = gen_jump (label1);
+		  new_jump = emit_jump_insn_after (new_jump_label,
+						   new_label_insn);
+		  INSN_ADDRESSES_NEW (new_jump, -1);
+		  JUMP_LABEL (new_jump) = new_jump_label;
+		  return;
+		}
+	      continue;
+	    }
+
+	  /* If the offset is too large, search for another candidate.  */
+	  if (!OK_FOR_MIPS16_BRANCH (label_off))
+	    continue;
+
+	  new_label = gen_label_rtx ();
+	  new_label_insn = emit_label_before (new_label, insn);
+	  redirect_jump (jump_insn, new_label, 0);
+	  new_jump_label = gen_jump (label1);
+	  new_jump = emit_jump_insn_after (new_jump_label, new_label_insn);
+	  INSN_ADDRESSES_NEW (new_jump, -1);
+	  JUMP_LABEL (new_jump) = new_jump_label;
+	  return;
+	}
+    }
+}
+
 /* A for_each_rtx callback for which DATA is a mips_lo_sum_offset hash table.
    Record every LO_SUM in *LOC.  */
 
@@ -15103,6 +15316,17 @@ 
   htab = htab_create (37, mips_lo_sum_offset_hash,
 		      mips_lo_sum_offset_eq, free);
 
+  /* Ensure that MIPS16 branches are within range.  */
+  if (TARGET_MIPS16)
+    {
+      for (insn = get_insns (); insn != 0; insn = NEXT_INSN (insn))
+        {
+	  if (simplejump_p (insn) || any_condjump_p (insn))
+	    mips16_check_branches (insn);
+	}
+      shorten_branches (get_insns ());
+    }
+
   /* Make a first pass over the instructions, recording all the LO_SUMs.  */
   for (insn = get_insns (); insn != 0; insn = NEXT_INSN (insn))
     FOR_EACH_SUBINSN (subinsn, insn)