On Tue, Jun 11, 2013 at 9:30 PM, Marc Glisse <marc.gli...@inria.fr> wrote: > On Tue, 11 Jun 2013, Jeff Law wrote: > >> On 06/09/13 10:25, Marc Glisse wrote: >>> >>> Hello, >>> >>> this patch removes some self-assignments. I don't know if this is the >>> best way, but it passes a bootstrap and the testsuite on >>> x86_64-linux-gnu. >>> >>> 2013-06-10 Marc Glisse <marc.gli...@inria.fr> >>> >>> PR tree-optimization/57361 >>> gcc/ >>> * tree-ssa-dse.c (dse_possible_dead_store_p): Handle >>> self-assignment. >>> >>> gcc/testsuite/ >>> * gcc.dg/tree-ssa/pr57361.c: New file. >> >> So dse_optimize_stmt will verify the statement does not have volatile >> operands. > > > operand_equal_p also does it, so we are well covered there ;-) > > >> However, it doesn't verify the statement does not potentially throw (think >> about a segfault on the store when async exceptions are enabled). I think >> you need to test for that explicitly. > > > Hmm, I am not at all familiar with that. Google drowns me in C# and > javascript links, and grepping through the sources only pointed me to the > -fasynchronous-unwind-tables flag. > > Richard noticed in the PR that expand_assignment already does: > > /* Optimize away no-op moves without side-effects. */ > if (operand_equal_p (to, from, 0)) > return; > > so it looks like the operand_equal_p test should be sufficient (or the > compiler already breaks that code). > > >> Is there some reason this won't work in tree-ssa-dce.c? That gets run >> more often and these stores may be preventing code motion opportunities, so >> getting them out of the IL stream as early as possible would be good. > > > In the first version of the patch: > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57303#c6 > I was doing it in fold_stmt_1, which should be called often enough. Richard > suggested DSE in the PR, but that might be because he had a more subtle test > in mind. I am certainly ok with moving it to DCE or anywhere else...
DSE looks like the right place to me as we are removing a store. Yes, DCE removes a limited set of stores as well, but the way we remove this kind of store makes it much more suited to DSE. As of possibly trapping/throwing stores, we do not bother to preserve those (even with -fnon-call-exceptions). Thus, the patch is ok with me if you agree with that and with the following adjustment: + /* Self-assignments are zombies. */ + if (gimple_assign_rhs_code (stmt) == TREE_CODE (gimple_assign_lhs (stmt)) + && operand_equal_p (gimple_assign_rhs1 (stmt), + gimple_assign_lhs (stmt), 0)) I see no need to compare the codes of the LHS and the RHS, that's redundant with what operand_equal_p does. Thus, ok with removing that test if it bootstraps / regtests ok and if Jeff has no further comments. Thanks, Richard. >> I'd be curious how often this triggers in GCC itself as well. > > > Do you know a convenient way to test that? > > -- > Marc Glisse
