On Thu, Feb 18, 2016 at 09:41:49AM +1030, Alan Modra wrote:
> On Wed, Feb 17, 2016 at 06:31:45AM -0600, Segher Boessenkool wrote:
> > > Corresponding content of "op" which causes the ICE:
> > > gdb) p debug_rtx (op)
> > > (gtu:SI (reg:CC 166) ---------------------- (operator and mode doesn't
> > > match)
> > > (const_int 0 [0]))
> >
> > That is invalid RTL for this target (should be CCUNS). Invalid RTL
> > should not be passed to recog.
>
> Really?? combine does that all the time, when it asks "is this
> instruction valid"!
No, it asks "is this valid RTL a valid insn for the target".
Some extreme examples... (plus:SI (reg:SI) (nil)) blows up.
(set:SI (reg:SI) (reg:SI)) is silently accepted.
> > > (gdb) p debug_rtx (other_insn)
> > > (insn 11 10 16 2 (set (reg:SI 165 [ D.2339+-3 ])
> > > (if_then_else:SI (ne (reg:CC 166)
> > > (const_int 0 [0]))
> > > (reg:SI 168)
> > > (reg:SI 167))) test.c:7 317 {isel_unsigned_si}
> > > (expr_list:REG_DEAD (reg:SI 168)
> > > (expr_list:REG_DEAD (reg:SI 167)
> > > (expr_list:REG_DEAD (reg:CC 166)
> > > (expr_list:REG_EQUAL (gtu:SI (reg:CC 166)
> > > (const_int 0 [0]))
> > > (nil))))))
> >
> > The REG_EQUAL there is bad already. Where does that come from?
>
> Rohit explain that quite well already, I thought. It's there due to
> combine transforming a GTU to NE in another insn, which means the reg
> mode changes to CCmode via rs6000.h:SELECT_CC_MODE.
I meant where in the code exactly, and of course what is the problem
there, fix *that*, because *that* is the problem. I have a patch btw.
> You might argue that combine shouldn't create such a note, but whether
> the note is valid or not depends on the target, doesn't it?
The note was valid. Then the mode of the psuedo was changed, and now the
note is no longer valid (or on some targets it still may be, targets that
allow multiple CC_MODEs for one and the same comparison+modes).
> And the
> usual way for combine to check validity of rtl is to form up an
> instruction and pass that to recog. Which is exactly what happens
> later when combine tries to use the note and runs into the rs6000
> backend assert.
No, that is not what happens.
The bad note is formed at an *earlier* combination (19->8 with current
trunk and the first testcase). That passes recog just fine -- and
correctly so -- but recog doesn't even get to see the notes at all.
There is nothing you can do in rs6000 code to stop this bug. You can
only hide the resulting crash.
Either combine should delete the note (my current patch), or it can
change the note to a copy of the new expression. My big worry is, can
there be a later insn that does not mention the reg in the pattern,
but *does* have it in its own REG_EQ*? I don't see how it could be
generated, but it's not really invalid either?
> It seems quite plain to me that this is primarily an rs6000 backend
> problem, solved by the blindingly obvious patch I posted. Whether you
> want to do something in combine as well is a secondary problem. The
> rs6000 backend shouldn't assert on this rtl.
The rs6000 backend *should* assert on it: it is invcalid RTL, it will
not work, it is meaningless. And in fact that assert *did* catch the
bug here!
Segher
