Re: [patch] Handle equivalent loop invariants as one in loop-invariant.c

[Zdenek Dvorak <rakdver at atrey dot karlin dot mff dot cuni dot cz>]

Hello,

> > > I don't see why you need to do the submode stuff here.  What if you
> > > just do
> > >     if (code != GET_CODE (e2)
> > >         || (mode1 != mode2
> > >             && mode1 != VOIDmode
> > >             && mode2 != VOIDmode))
> > > at the start?  It doesn't seem like you are using the mode for
> > > anything substantial.
> > 
> > this seems possible, but I somewhat prefer the current solution, where
> > the arguments MODE1 and MODE2 always tell you mode of the expressions E1 and
> > E2 (i.e., they are never VOIDmode).  With the proposed change, it is no
> > longer true (you may get VOIDmode in both Ei and MODEi).  The code still works
> > correctly, but only because you know that this will only happen in the
> > case invariant_expr_equal_p is called recursively and the modes were
> > checked there; which gives much more room for introducing errors upon
> > later changes.
> 
> I don't really see how.  The only way that both Ei and MODEi could be
> VOIDmode would be if a CONST_INT were compared to a CONST_INT.  And
> the mode of a CONST_INT doesn't matter.  It is effectively the mode of
> the parent.  And, as you say, the code has already checked that.

yes; and it is bad to rely on what someone might (or might not) do.

> There is code all over the compiler which does this sort of
> equivalency checking, but doesn't bother to do the submode code.
> Introducing it only here requires special justification--why does this
> code require the extra attention?

huh?  We do this (keeping account of what mode the VOIDmode operand
really is) all over the compiler; there is nothing special about this
code.

Anyway, I don't really care.  I dislike the solution you propose,
because its semantics is just weird; but not passing MODE1 and MODE2 to
the function at all and requiring the caller to verify that the modes
match is equivalent and somewhat cleaner (although a bit cumbersome to
use).  I will prepare a patch with this change, if you insist.

Zdenek