Patchwork [mips] Fix for PR target/56942

login
register
mail settings
Submitter Steve Ellcey
Date April 19, 2013, 10:35 p.m.
Message ID <2c243f55-9b58-4e4a-86cf-9dd5be8ea183@BAMAIL02.ba.imgtec.org>
Download mbox | patch
Permalink /patch/238127/
State New
Headers show

Comments

Steve Ellcey - April 19, 2013, 10:35 p.m.
Andrew Bennett found this fix to my MIPS build problem (PR target/56942).
He does not have write access so I am submitting it for checkin, is this
a simple enough fix for an 'obvious' checkin?  I did a build and regression
test to verify the fix.

Steve Ellcey
sellcey@imgtec.com



2013-04-19  Andrew Bennett <andrew.bennett@imgtec.com>
	    Steve Ellcey  <sellcey@imgtec.com>

	PR target/56942
	* config/mips/mips.md (casesi_internal_mips16_<mode>): Use
	next_active_insn instead of next_real_insn.
Richard Sandiford - April 24, 2013, 6:45 a.m.
"Steve Ellcey " <sellcey@imgtec.com> writes:
> 2013-04-19  Andrew Bennett <andrew.bennett@imgtec.com>
> 	    Steve Ellcey  <sellcey@imgtec.com>
>
> 	PR target/56942
> 	* config/mips/mips.md (casesi_internal_mips16_<mode>): Use
> 	next_active_insn instead of next_real_insn.

Hmm, I don't really like this.  Steven said from ARM in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56809:

-----------------------------------------------------------------------
Target bug, this is wrong:

  rtx diff_vec = PATTERN (next_real_insn (operands[2]));

A jump_table_data is not a real insn.  Before my patch this worked
by accident because the jump table would hide in a JUMP_INSN and 
next_real_insn returned any JUMP_P insn.

Use next_active_insn instead.
-----------------------------------------------------------------------

But using next_real_insn was at least as correct (IMO, more correct)
as next_active_insn before r197266.  It seems counterintuitive that
something can be "active" but not "real".

Richard
Steve Ellcey - April 26, 2013, 10:41 p.m.
On Wed, 2013-04-24 at 07:45 +0100, Richard Sandiford wrote:
> "Steve Ellcey " <sellcey@imgtec.com> writes:
> > 2013-04-19  Andrew Bennett <andrew.bennett@imgtec.com>
> > 	    Steve Ellcey  <sellcey@imgtec.com>
> >
> > 	PR target/56942
> > 	* config/mips/mips.md (casesi_internal_mips16_<mode>): Use
> > 	next_active_insn instead of next_real_insn.
> 
> Hmm, I don't really like this.  Steven said from ARM in
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56809:
> 
> -----------------------------------------------------------------------
> Target bug, this is wrong:
> 
>   rtx diff_vec = PATTERN (next_real_insn (operands[2]));
> 
> A jump_table_data is not a real insn.  Before my patch this worked
> by accident because the jump table would hide in a JUMP_INSN and 
> next_real_insn returned any JUMP_P insn.
> 
> Use next_active_insn instead.
> -----------------------------------------------------------------------
> 
> But using next_real_insn was at least as correct (IMO, more correct)
> as next_active_insn before r197266.  It seems counterintuitive that
> something can be "active" but not "real".
> 
> Richard

So should we put the active_insn_p hack/FIXME into real_next_insn?  That
doesn't seem like much of a win but it would probably fix the problem.

Steve Ellcey
sellcey@imgtec.com



>From emit-rtl.c:

/* Find the next insn after INSN that really does something.  This routine
   does not look inside SEQUENCEs.  After reload this also skips over
   standalone USE and CLOBBER insn.  */

int
active_insn_p (const_rtx insn)
{
  return (CALL_P (insn) || JUMP_P (insn)
          || JUMP_TABLE_DATA_P (insn) /* FIXME */
          || (NONJUMP_INSN_P (insn)
              && (! reload_completed
                  || (GET_CODE (PATTERN (insn)) != USE
                      && GET_CODE (PATTERN (insn)) != CLOBBER))));
}
Richard Sandiford - April 27, 2013, 7:56 a.m.
Steve Ellcey <sellcey@imgtec.com> writes:
> On Wed, 2013-04-24 at 07:45 +0100, Richard Sandiford wrote:
>> "Steve Ellcey " <sellcey@imgtec.com> writes:
>> > 2013-04-19  Andrew Bennett <andrew.bennett@imgtec.com>
>> > 	    Steve Ellcey  <sellcey@imgtec.com>
>> >
>> > 	PR target/56942
>> > 	* config/mips/mips.md (casesi_internal_mips16_<mode>): Use
>> > 	next_active_insn instead of next_real_insn.
>> 
>> Hmm, I don't really like this.  Steven said from ARM in
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=56809:
>> 
>> -----------------------------------------------------------------------
>> Target bug, this is wrong:
>> 
>>   rtx diff_vec = PATTERN (next_real_insn (operands[2]));
>> 
>> A jump_table_data is not a real insn.  Before my patch this worked
>> by accident because the jump table would hide in a JUMP_INSN and 
>> next_real_insn returned any JUMP_P insn.
>> 
>> Use next_active_insn instead.
>> -----------------------------------------------------------------------
>> 
>> But using next_real_insn was at least as correct (IMO, more correct)
>> as next_active_insn before r197266.  It seems counterintuitive that
>> something can be "active" but not "real".
>> 
>> Richard
>
> So should we put the active_insn_p hack/FIXME into real_next_insn?  That
> doesn't seem like much of a win but it would probably fix the problem.

Yeah, I think so.  If "=>" mean "accepts more than", then there used
to be a nice total order:

     next_insn
  => next_nonnote_insn
  => next_real_insn
  => next_active_insn

I think we should keep that if possible, even during the transition period.

Thanks,
Richard

Patch

diff --git a/gcc/config/mips/mips.md b/gcc/config/mips/mips.md
index 2f62910..caf4614 100644
--- a/gcc/config/mips/mips.md
+++ b/gcc/config/mips/mips.md
@@ -6019,7 +6019,7 @@ 
    (clobber (reg:SI MIPS16_T_REGNUM))]
   "TARGET_MIPS16_SHORT_JUMP_TABLES"
 {
-  rtx diff_vec = PATTERN (next_real_insn (operands[2]));
+  rtx diff_vec = PATTERN (next_active_insn (operands[2]));
 
   gcc_assert (GET_CODE (diff_vec) == ADDR_DIFF_VEC);