diff mbox

[mips] Fix for PR target/56942

Message ID 1367266413.8625.3.camel@ubuntu-sellcey
State New
Headers show

Commit Message

Steve Ellcey April 29, 2013, 8:13 p.m. UTC 
On Sat, 2013-04-27 at 08:56 +0100, Richard Sandiford wrote:

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

OK, here is patch to next_real_insn to keep the ordering property intact
and fix the bug.  OK for checkin?

Steve Ellcey
sellcey@imgtec.com



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

	PR target/56942
	* emit-rtl.c (next_real_insn): Accept jump table data
	as 'real' (like next_active_insn does).

Comments

Richard Sandiford April 30, 2013, 6:57 a.m. UTC | #1 
Steve Ellcey <sellcey@imgtec.com> writes:
> OK, here is patch to next_real_insn to keep the ordering property intact
> and fix the bug.  OK for checkin?

Thanks, looks good to me, but an rtl/middle-end/global maintainer
would need to approve it.

Richard
Steven Bosscher April 30, 2013, 11:46 a.m. UTC | #2 
(Top post is gmail's fault ;-)

Hello,

I dont like this at all.      At the very least, if we go this way,
then all places where next_active_insn is used should be updated.
Otherwise this is just confusion proliferation. Before my patch most
ports used the "active" variants and I specifically did non fix the
"real" variants. It is marked fixme for a reason: The JUMP_TABLE_DATA
should always follow immediately after the label. Copying the fixme is
a step in the wrong direction. Please do not commit this patch!

Ciao!
Steven



On 4/30/13, Richard Sandiford <rdsandiford@googlemail.com> wrote:
> Steve Ellcey <sellcey@imgtec.com> writes:
>> OK, here is patch to next_real_insn to keep the ordering property intact
>> and fix the bug.  OK for checkin?
>
> Thanks, looks good to me, but an rtl/middle-end/global maintainer
> would need to approve it.
>
> Richard
>
Richard Sandiford April 30, 2013, 1:28 p.m. UTC | #3 
Steven Bosscher <stevenb.gcc@gmail.com> writes:
> I dont like this at all.      At the very least, if we go this way,
> then all places where next_active_insn is used should be updated.
> Otherwise this is just confusion proliferation.

You mean all places where next_active_insn is used to get the jump table?
That would be fine with me, but as author of the original change,
I'm going to ask you to do that if you feel strongly about it :-)
Otherwise Steve's patch seems fine to me.

> Before my patch most
> ports used the "active" variants and I specifically did non fix the
> "real" variants. It is marked fixme for a reason: The JUMP_TABLE_DATA
> should always follow immediately after the label. Copying the fixme is
> a step in the wrong direction. Please do not commit this patch!

But you didn't respond to my main point.  It always used to be the
case that all "active" insns were also "real".  I.e. "real" was a
_more_ restrictive condition than "active".  Having insns that are
"active" but not "real" is a change to the interface and also
(IMO) doesn't make much sense in terms of English usage.

Don't get me wrong: I like the change to use something other
than JUMP_INSN to store the jump table, and thanks for making it.
I just don't think we should "break" the next_*_insn hierachy at
the same time.

Richard
Richard Sandiford April 30, 2013, 2:05 p.m. UTC | #4 
Richard Sandiford <rdsandiford@googlemail.com> writes:
> Steven Bosscher <stevenb.gcc@gmail.com> writes:
>> I dont like this at all.      At the very least, if we go this way,
>> then all places where next_active_insn is used should be updated.
>> Otherwise this is just confusion proliferation.
>
> You mean all places where next_active_insn is used to get the jump table?
> That would be fine with me, but as author of the original change,
> I'm going to ask you to do that if you feel strongly about it :-)
> Otherwise Steve's patch seems fine to me.
>
>> Before my patch most
>> ports used the "active" variants and I specifically did non fix the
>> "real" variants. It is marked fixme for a reason: The JUMP_TABLE_DATA
>> should always follow immediately after the label. Copying the fixme is
>> a step in the wrong direction. Please do not commit this patch!
>
> But you didn't respond to my main point.  It always used to be the
> case that all "active" insns were also "real".  I.e. "real" was a
> _more_ restrictive condition than "active".

Gah, I really wish proof reads before hitting "send" were as effective
as those after.  Obviously that should read: "active" was a _more_
restrictive condition than "real".

Richard
Steve Ellcey May 3, 2013, 8:47 p.m. UTC | #5 
On Tue, 2013-04-30 at 15:05 +0100, Richard Sandiford wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
> > Steven Bosscher <stevenb.gcc@gmail.com> writes:
> >> I dont like this at all.      At the very least, if we go this way,
> >> then all places where next_active_insn is used should be updated.
> >> Otherwise this is just confusion proliferation.
> >
> > You mean all places where next_active_insn is used to get the jump table?
> > That would be fine with me, but as author of the original change,
> > I'm going to ask you to do that if you feel strongly about it :-)
> > Otherwise Steve's patch seems fine to me.
> >
> >> Before my patch most
> >> ports used the "active" variants and I specifically did non fix the
> >> "real" variants. It is marked fixme for a reason: The JUMP_TABLE_DATA
> >> should always follow immediately after the label. Copying the fixme is
> >> a step in the wrong direction. Please do not commit this patch!
> >
> > But you didn't respond to my main point.  It always used to be the
> > case that all "active" insns were also "real".  I.e. "real" was a
> > _more_ restrictive condition than "active".
> 
> Gah, I really wish proof reads before hitting "send" were as effective
> as those after.  Obviously that should read: "active" was a _more_
> restrictive condition than "real".
> 
> Richard

Steven,

Do you have any comment on Richard's question?  I am basically stuck
between the two of you with no way to fix my problem at this point
unless you allow the generic fix or Richard allows the MIPS specific
fix.

Steve Ellcey
sellcey@imgtec.com
Steven Bosscher May 4, 2013, 5:12 a.m. UTC | #6 
Hello,

I'm on holiday, but I'm back home tomorrow.

Imho the active-insn "idiom" is the best solution for the moment. I
will fix this mess properly asap, probably next week.

Ciao!
Steven



On 5/3/13, Steve Ellcey <sellcey@imgtec.com> wrote:
> On Tue, 2013-04-30 at 15:05 +0100, Richard Sandiford wrote:
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> > Steven Bosscher <stevenb.gcc@gmail.com> writes:
>> >> I dont like this at all.      At the very least, if we go this way,
>> >> then all places where next_active_insn is used should be updated.
>> >> Otherwise this is just confusion proliferation.
>> >
>> > You mean all places where next_active_insn is used to get the jump
>> > table?
>> > That would be fine with me, but as author of the original change,
>> > I'm going to ask you to do that if you feel strongly about it :-)
>> > Otherwise Steve's patch seems fine to me.
>> >
>> >> Before my patch most
>> >> ports used the "active" variants and I specifically did non fix the
>> >> "real" variants. It is marked fixme for a reason: The JUMP_TABLE_DATA
>> >> should always follow immediately after the label. Copying the fixme is
>> >> a step in the wrong direction. Please do not commit this patch!
>> >
>> > But you didn't respond to my main point.  It always used to be the
>> > case that all "active" insns were also "real".  I.e. "real" was a
>> > _more_ restrictive condition than "active".
>>
>> Gah, I really wish proof reads before hitting "send" were as effective
>> as those after.  Obviously that should read: "active" was a _more_
>> restrictive condition than "real".
>>
>> Richard
>
> Steven,
>
> Do you have any comment on Richard's question?  I am basically stuck
> between the two of you with no way to fix my problem at this point
> unless you allow the generic fix or Richard allows the MIPS specific
> fix.
>
> Steve Ellcey
> sellcey@imgtec.com
>
>
>
>
>
Richard Sandiford May 28, 2013, 7:36 p.m. UTC | #7 
Hi Steven,

Steven Bosscher <stevenb.gcc@gmail.com> writes:
> Imho the active-insn "idiom" is the best solution for the moment. I
> will fix this mess properly asap, probably next week.

Just wondering, how are things going with this?  (I assume fixing it
properly means getting rid of the FIXME in next_active_insn?)

Thanks,
Richard
Steven Bosscher May 28, 2013, 8:39 p.m. UTC | #8 
On Tue, May 28, 2013 at 9:36 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Hi Steven,
>
> Steven Bosscher <stevenb.gcc@gmail.com> writes:
>> Imho the active-insn "idiom" is the best solution for the moment. I
>> will fix this mess properly asap, probably next week.
>
> Just wondering, how are things going with this?  (I assume fixing it
> properly means getting rid of the FIXME in next_active_insn?)

Yes, it involves getting this to work:

-------------------------
 bool
 tablejump_p (const_rtx insn, rtx *labelp, rtx *tablep)
 {
   rtx label, table;

   if (!JUMP_P (insn))
     return false;

   label = JUMP_LABEL (insn);
   if (label != NULL_RTX && !ANY_RETURN_P (label)
       && (table = next_active_insn (label)) != NULL_RTX
       && JUMP_TABLE_DATA_P (table))
     {
+      gcc_assert (table == NEXT_INSN (label));
       if (labelp)
        *labelp = label;
       if (tablep)
        *tablep = table;
       return true;
     }
   return false;
 }
-------------------------

and then going over the places where next_*_insn is used to get the table.

The trouble is that I haven't yet found a case where the above does
*not* work. Help welcome.

Ciao!
Steven
diff mbox

Patch

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 538b1ec..9de3f1e 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -3248,7 +3248,8 @@  next_real_insn (rtx insn)
   while (insn)
     {
       insn = NEXT_INSN (insn);
-      if (insn == 0 || INSN_P (insn))
+      if (insn == 0 || INSN_P (insn)
+	  || JUMP_TABLE_DATA_P (insn)) /* FIXME */
 	break;
     }