Re: strength reduction example

[Jim Wilson <wilson@cygnus.com>]

I understand what the patch is doing now.

I am still concerned that you are calling subst from loop.  You are doing
it indirectly through validate_subst which is much better, but you are still
calling it from loop.  This has implications for long term maintenance,
since it means anyone modifying combine has to remember that the code must
work called from loop also.  It is likely that people will forget this, and
we will see occasional bugs as a result.  I don't see any easy convenient
alternative though, and the combine changes to make this work seem pretty
simple, so it appears to be worth trying.

The current code always does this:
            VARRAY_CHAR (may_not_optimize, new_regno) = 0;
The patched code only does this if an increment is eliminated.  I don't
understand how that can be right unless the current code is actually wrong.

Before the for loop that calls validate_subst, you should add a comment
explaining that we try to create address givs by replacing reg uses
with (PLUS reg increment).

The patch calls validate_subst for every instruction, regardless of whether
it uses old_reg.  validate_subst calls subst and recog for every instruction
regardless of whether any change was made.  This seems wasteful.  Shouldn't
we call reg_mentioned_p first, and if it returns true, only then call
validate_subst?  That might not work if there is a REG_EQUAL note that
refers to old_reg but the insn doesn't.  Perhaps the reg_mentioned_p check
should be in validate_subst?

Since we really want to create address givs, maybe we should only do this
substitution inside MEMs?  Will something useful happen if it gets replaced
elsewhere?  I guess that will result in creation of a DEST_REG giv, so it
should still work.

Typo in combine.c: sucess -> success.

The code in validate_subst to set previous_undos from undos looks unnecessary.
previous_undos is only used if we call subst multiple times.  If we want
to undo the effect of the last subst call, then we can restore undos from
previous_undos.  This doesn't apply to validate_subst.

validate_subst assumes that undobuf.undos has some reasonable value at
the beginning.  I don't think that is a safe assumption.  You should set
undobuf.undos to zero at the beginning.

Why do you call validate_change in validate_subst?  It would be simpler to
call recog_insn.  You could then get rid of 3 other statements that fiddle
wtih PATTERN (insn).  The only reason I see for this is because validate_change
tries adding/removing clobbers.  In that case, you should call
recog_for_combine.  It looks safe, and will probably fix a bug, since
recog_for_combine looks for the (clobber (const_int 0)) stuff that
combine sometimes creates for invalid substitutions.  These clobbers would
just be ignored by validate_change.

validate_subst only tries to fix a REG_EQUAL note if validate_change
succeeded.  But what if we have an insn that doesn't use the register
but has a REG_EQUAL note that does?  This could happen for a libcall
sequence.  It might be possible in other cases.  In loop, the
DEST_REG giv case immediately after the validate_subst call handles
reg notes independently.  Either the code in validate_subst is wrong,
or the code in loop is unnecessarily inefficient.  I think the validate_subst
code is wrong.

Jim