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

[Ian Lance Taylor <ian at airs dot com>]

Zdenek Dvorak <rakdver@atrey.karlin.mff.cuni.cz> writes:

> > > +/* Finds the reference corresponding to the use of REG in INSN.
> > > +   DF is the dataflow object.  */
> > > +
> > > +struct ref *
> > > +df_find_use (struct df *df, rtx insn, rtx reg)
> > > +{
> > > +  struct df_link *uses;
> > > +
> > > +  for (uses = DF_INSN_USES (df, insn); uses; uses = uses->next)
> > > +    {
> > > +      rtx areg = DF_REF_REG (uses->ref);
> > > +      if (GET_CODE (areg) == SUBREG)
> > > +	areg = SUBREG_REG (areg);
> > > +
> > > +      if (rtx_equal_p (areg, reg))
> > > +	return uses->ref;
> > > +    }
> > > +
> > > +  return NULL;
> > > +}
> > > +
> > 
> > This seems more or less parallel to df_find_def and df_reg_used, but
> > those functions don't check for a SUBREG.  It seems to me that the
> > functions should be entirely parallel; if it right to check for a
> > SUBREG here, then presumably it is right to check for one in
> > df_find_def.
> 
> this probably is true, although it needs testing.

Well, obviously, it depends on how they are used.

In the patch which Steven sent, I don't think this new function will
ever get called with a SUBREG.  So it might be simpler to simply not
check for SUBREG here.  I don't see what the advantage is of checking
for SUBREG.

I personally don't care, as long as the functions are parallel.  If
anybody else has an opinion, please express it.

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

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?  Otherwise we should keep the code
similar to other cases.

Thanks.

Ian