> From: Jeff Law [mailto:l...@redhat.com]
> Sent: Tuesday, May 12, 2015 4:17 AM
>
> On 05/06/2015 03:47 AM, Thomas Preud'homme wrote:
> > Ping?
> Something to consider as future work -- I'm pretty sure PRE sets up the
> same kind of problematical pattern with a new pseudo (reaching reg)
> holding the result of the redundant expression and the original
> evaluations turned into copies from the reaching reg to the final
> destination.
Yes absolutely, this is how the pattern I was interested in was created. The
reason I solved it in loop-invariant is that I thought this was on purpose
with the cleanup left to loop-invariant. When finding a TODO comment
about this in loop-invariant I thought it confirmed my initial thoughts.
>
> That style is easy to prove correct. There was an issue with the copies
> not propagating away that was pretty inherent in the partial redundancy
> cases that I could probably dig out of my archives if you're interested.
If you think this should also (or instead) be fixed in PRE I can take a look
at some point later since it shouldn't be much more work.
> It looks like there's a variety of line wrapping issues. Please
> double-check line wrapping using an 80 column window. Minor I know,
> but
> the consistency with the rest of the code is good.
Looking in vim seems to systematically cut at 80 column and
check_GNU_style.sh only complain about the dg-final line in the
new testcases. Could you point me to such an occurrence?
>
> >>
> >> +
> >> + /* Check that all uses reached by the def in insn would still be
> reached
> >> + it. */
> >> + dest_regno = REGNO (reg);
> >> + for (use = DF_REG_USE_CHAIN (dest_regno); use; use =
> >> DF_REF_NEXT_REG (use))
> [ ... ]
> So isn't this overly conservative if DEST_REGNO is set multiple times
> since it's going to look at all the uses, even those not necessarily
> reached by the original SET of DEST_REGNO?
>
> Or is that not an issue for some reason? And I'm not requiring you to
> make this optimal, but if I'm right, a comment here seems wise.
My apologize, it is the comment that is incorrect since it doesn't match
the code (a remaining of an old version of this patch). The code actually
checks that the use was dominated by the instruction before it is moved
out of the loop. This is to prevent the code motion in case like:
foo = 1;
bar = 0;
for ()
{
bar += foo;
foo = 42;
}
which I met in some of the testsuite cases.
>
>
> I think with the wrapping nits fixed and closure on the multi-set issue
> noted immediately above and this will be good for the trunk.
I'll fix this comment right away.
Best regards,
Thomas
