diff mbox series

tree-ssa-dse: Fix up handling of lhs of internal calls [PR108657]

Message ID Y+3vzqcqv9AZsKy+@tucnak
State Unresolved
Headers
Series tree-ssa-dse: Fix up handling of lhs of internal calls [PR108657] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jakub Jelinek Feb. 16, 2023, 8:56 a.m. UTC 
  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?

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?

2023-02-15  Jakub Jelinek  <jakub@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.


	Jakub

Comments

Richard Biener Feb. 16, 2023, 2:21 p.m. UTC | #1 
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  <jakub@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
> 
>
Jakub Jelinek Feb. 16, 2023, 2:31 p.m. UTC | #2 
On Thu, Feb 16, 2023 at 02:21:04PM +0000, Richard Biener wrote:
> > 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.

Yes.

	Jakub
diff mbox series

Patch

--- 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 ();
+}