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)

Reply via email to