On 05/12/2015 02:55 AM, Thomas Preud'homme wrote:
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.
My memory is quite fuzzy, but IIRC there wasn't a really good way to fix
this within PRE and it was one of the reasons why we left in the old
classical gcse for so long (which was used for -Os as it didn't
introduce the partially redundant copies).
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.
My recollection was that it wasn't reasonably fixed in PRE. Some kind
of copy motion or PRE on copies might help.
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?
Strangely enough, looking at the patch you just sent, I don't see any
long line issues. Maybe I was looking at an old or the wrong patch.
+
+ /* 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:
Ah. Thanks for the clarification and fixing up the comment.
I'll take a final looksie shortly.
jeff
