On Thu, 16 Feb 2023, Jakub Jelinek wrote: > Hi! > > The r13-1778 PR106378 tree-ssa-dse change didn't just add special support > for IFN_LEN_STORE and IFN_MASK_STORE internal function calls as I believe > was intended, but given that the function was > if (is builtin) { ... } > else if (lhs present and non-SSA_NAME) { ... } > return false; > and it added a new > else if (is internal builtin) { ... } > in between the two, the last if used to be done before on all stmts > with non-SSA_NAME lhs except for calls to builtin functions, but newly > isn't done also for calls to internal functions. In the testcase > the important internal function is .DEFERRED_INIT, which often has > non-SSA_NAME lhs, and the change resulted in them no longer being DSEd, > so a block with nothing in it left but var = .DEFERRED_INIT () and > var = {CLOBBER} was unrolled several times. > > The following patch does the lhs handling for all stmts with non-SSA_NAME lhs > unless initialize_ao_ref_for_dse handled those specially already and > returned (which is the case for various mem* builtins which don't have > such lhs, for some cases of calloc which again is fine,and since r13-1778 > also for IFN_LEN_STORE call and some IFN_MASK_STORE calls. > As IFN_MASK_STORE doesn't have a lhs, the break for the !may_def_ok case > doesn't seem to change anything, and because we've handled internal fns > that way in the past, I think it is the right thing to do that again. > That said, if it is inappropriate for some new ifn, I guess it could > be added to the switch and just return false; for it instead of break;. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK. > That said, while this patch fixes the regression by allowing DSE of > IFN_DEFERRED_INIT again, I think we probably have some latent bug in FRE > where without this patch it seems to be fre5 that sees one unconditional > c = 1; store, one conditional c = 0; store and in the last bb before return > another c = 1; store and decides that the last store is redundant, which is > not the case, the first two stores are redundant or if they can't be > removed, none of them is. Richard, could you please have a look? That's before this patch only? I'll have a look. Thanks, Richard. > 2023-02-15 Jakub Jelinek <ja...@redhat.com> > > PR tree-optimization/108657 > * tree-ssa-dse.cc (initialize_ao_ref_for_dse): If lhs of stmt > exists and is not a SSA_NAME, call ao_ref_init even if the stmt > is a call to internal or builtin function. > > * gcc.dg/pr108657.c: New test. > > --- gcc/tree-ssa-dse.cc.jj 2023-01-11 10:29:08.651161134 +0100 > +++ gcc/tree-ssa-dse.cc 2023-02-15 20:03:33.647684713 +0100 > @@ -177,7 +177,7 @@ initialize_ao_ref_for_dse (gimple *stmt, > default:; > } > } > - else if (tree lhs = gimple_get_lhs (stmt)) > + if (tree lhs = gimple_get_lhs (stmt)) > { > if (TREE_CODE (lhs) != SSA_NAME) > { > --- gcc/testsuite/gcc.dg/pr108657.c.jj 2023-02-15 20:11:22.038804168 > +0100 > +++ gcc/testsuite/gcc.dg/pr108657.c 2023-02-15 20:10:37.992451199 +0100 > @@ -0,0 +1,31 @@ > +/* PR tree-optimization/108657 */ > +/* { dg-do run } */ > +/* { dg-options "-O3 -ftrivial-auto-var-init=zero" } */ > + > +int c, e, f; > +static int *d = &c; > + > +__attribute__((noipa)) void > +foo (void) > +{ > + if (c != 1) > + __builtin_abort (); > +} > + > +int > +main () > +{ > + for (c = 1; c >= 0; c--) > + { > + e = 0; > + for (int j = 0; j <= 2; j++) > + { > + short k[1]; > + if (e) > + break; > + e ^= f; > + } > + } > + *d = 1; > + foo (); > +} > > Jakub > > -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)