On Wed, Aug 20, 2014 at 5:05 PM, Marc Glisse <marc.gli...@inria.fr> wrote: > On Wed, 20 Aug 2014, Richard Biener wrote: > >> On Wed, Aug 20, 2014 at 4:18 PM, Marc Glisse <marc.gli...@inria.fr> wrote: >>> >>> On Wed, 20 Aug 2014, Richard Biener wrote: >>> >>>>>>> - if (stmt != use_stmt >>>>>>> - && ref_maybe_used_by_stmt_p (use_stmt, gimple_assign_lhs >>>>>>> (stmt))) >>>>>>> - return; >>>>>>> - >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> I don't see how you can remove this code? >>>>> >>>>> >>>>> >>>>> >>>>> dse_possible_dead_store_p already tests ref_maybe_used_by_stmt_p and >>>>> thus cannot return true with such a use_stmt, as far as I can see. As I >>>>> said, bootstrap+testsuite with an assert instead of 'return' didn't >>>>> turn >>>>> up anything. I could keep it as a gcc_checking_assert (with a slight >>>>> update to the comment) if you like. Or did I miss a path in >>>>> dse_possible_dead_store_p? >>>> >>>> >>>> >>>> Yes, the one that early returns from the operand_equal_p check. >>>> >>>> You might want to do some svn blaming to see what testcases >>>> were added with the above code (and the code surrounding it). >>>> >>>> I'm not sure either... so if it passes bootstrap & regtest it must be >>>> dead code... (well...) >>> >>> >>> >>> The early return operand_equal_p has use_stmt == stmt, so it doesn't even >>> reach the call to ref_maybe_used_by_stmt_p I am removing. >>> >>> svn blame leads me to r132899 (gcc.c-torture/execute/pr35472.c) >>> and before that to r131101 (gcc.c-torture/execute/20071219-1.c) >> >> >> Maybe add some noclone attributes as well? > > > Ok, both testcases now have noinline,noclone instead of just noinline in my > tree. > > * gcc.c-torture/execute/pr35472.c: Add noclone attribute. > * gcc.c-torture/execute/20071219-1.c: Likewise. > >> And try -fno-tree-fre/pre? > > > No problem. > (it makes it harder to find a killing stmt) > > >> But pr35472.c clearly shows what it was trying to fix: >> >> *p = a; >> *p = b; >> >> with p == &b. The *p = b; store does _not_ make *p = a dead because >> *p = b; is a no-op. So a modified testcase would be >> >> *p = a; >> *p = *p; >> >> where without your patch the first DSE pass removes *p = *p (but >> not first *p = a). >> >> Maybe if that still works after your patch you can add this as a new >> testcase, >> also verifying that *p = *p store indeed is removed. > > > DSE goes backwards, so it first removes *p=*p (already tested by > gcc.dg/tree-ssa/pr57361.c) and when we look at *p=*a; there is no second > statement left, so it wouldn't really test what you want.
Ah, ok. Well, I suppose the patch is ok with the check removed then. Thanks, Richard. > -- > Marc Glisse
