This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Handle equivalent loop invariants as one in loop-invariant.c
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