Message ID | 1367266413.8625.3.camel@ubuntu-sellcey |
---|---|
State | New |
Headers | show |
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
(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 >
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 <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
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
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 > > > > >
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
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 --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; }