diff mbox series

reassoc: Fix up another ICE with returns_twice call [PR109410]

Message ID ZDbB+n2lzHZoH6q5@tucnak
State Unresolved
Headers
Series reassoc: Fix up another ICE with returns_twice call [PR109410] |

Checks

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

Commit Message

Jakub Jelinek April 12, 2023, 2:36 p.m. UTC 
  Hi!

The following testcase ICEs in reassoc, unlike the last case I've fixed
there here SSA_NAME_USED_IN_ABNORMAL_PHI is not the case anywhere.
build_and_add_sum places new statements after the later appearing definition
of an operand but if both operands are default defs or constants, we place
statement at the start of the function.

If the very first statement of a function is a call to returns_twice
function, this doesn't work though, because that call has to be the first
thing in its basic block, so the following patch splits the entry successor
edge such that the new statements are added into a different block from the
returns_twice call.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

I think we should in stage1 reconsider such placements, I think it
unnecessarily enlarges the lifetime of the new lhs if its operand(s)
are used more than once in the function.  Unless something sinks those
again.  Would be nice to place it closer to the actual uses (or where
they will be placed).

2023-04-12  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/109410
	* tree-ssa-reassoc.cc (build_and_add_sum): Split edge from entry
	block if first statement of the function is a call to returns_twice
	function.

	* gcc.dg/pr109410.c: New test.


	Jakub

Comments

Richard Biener April 12, 2023, 2:50 p.m. UTC | #1 
> Am 12.04.2023 um 16:37 schrieb Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org>:
> 
> Hi!
> 
> The following testcase ICEs in reassoc, unlike the last case I've fixed
> there here SSA_NAME_USED_IN_ABNORMAL_PHI is not the case anywhere.
> build_and_add_sum places new statements after the later appearing definition
> of an operand but if both operands are default defs or constants, we place
> statement at the start of the function.
> 
> If the very first statement of a function is a call to returns_twice
> function, this doesn't work though, because that call has to be the first
> thing in its basic block, so the following patch splits the entry successor
> edge such that the new statements are added into a different block from the
> returns_twice call.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

> I think we should in stage1 reconsider such placements, I think it
> unnecessarily enlarges the lifetime of the new lhs if its operand(s)
> are used more than once in the function.  Unless something sinks those
> again.  Would be nice to place it closer to the actual uses (or where
> they will be placed).

IStR there is code to do that in reassoc already.

> 
> 2023-04-12  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR tree-optimization/109410
>    * tree-ssa-reassoc.cc (build_and_add_sum): Split edge from entry
>    block if first statement of the function is a call to returns_twice
>    function.
> 
>    * gcc.dg/pr109410.c: New test.
> 
> --- gcc/tree-ssa-reassoc.cc.jj    2023-02-18 12:40:42.739131728 +0100
> +++ gcc/tree-ssa-reassoc.cc    2023-04-12 13:23:49.083979843 +0200
> @@ -1564,6 +1564,15 @@ build_and_add_sum (tree type, tree op1,
>       && (!op2def || gimple_nop_p (op2def)))
>     {
>       gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
> +      if (!gsi_end_p (gsi)
> +      && is_gimple_call (gsi_stmt (gsi))
> +      && (gimple_call_flags (gsi_stmt (gsi)) & ECF_RETURNS_TWICE))
> +    {
> +      /* Don't add statements before a returns_twice call at the start
> +         of a function.  */
> +      split_edge (single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
> +      gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
> +    }
>       if (gsi_end_p (gsi))
>    {
>      gimple_stmt_iterator gsi2
> --- gcc/testsuite/gcc.dg/pr109410.c.jj    2023-04-12 13:42:41.759751843 +0200
> +++ gcc/testsuite/gcc.dg/pr109410.c    2023-04-12 13:42:27.249959585 +0200
> @@ -0,0 +1,19 @@
> +/* PR tree-optimization/109410 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +__attribute__((returns_twice)) int baz (int, int);
> +
> +int
> +bar (int x)
> +{
> +  return x;
> +}
> +
> +int
> +foo (int x, int y)
> +{
> +  baz (x, y);
> +  int a = bar (x);
> +  return y || a == 42 || a > 42;
> +}
> 
>    Jakub
>
diff mbox series

Patch

--- gcc/tree-ssa-reassoc.cc.jj	2023-02-18 12:40:42.739131728 +0100
+++ gcc/tree-ssa-reassoc.cc	2023-04-12 13:23:49.083979843 +0200
@@ -1564,6 +1564,15 @@  build_and_add_sum (tree type, tree op1,
       && (!op2def || gimple_nop_p (op2def)))
     {
       gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
+      if (!gsi_end_p (gsi)
+	  && is_gimple_call (gsi_stmt (gsi))
+	  && (gimple_call_flags (gsi_stmt (gsi)) & ECF_RETURNS_TWICE))
+	{
+	  /* Don't add statements before a returns_twice call at the start
+	     of a function.  */
+	  split_edge (single_succ_edge (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
+	  gsi = gsi_after_labels (single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
+	}
       if (gsi_end_p (gsi))
 	{
 	  gimple_stmt_iterator gsi2
--- gcc/testsuite/gcc.dg/pr109410.c.jj	2023-04-12 13:42:41.759751843 +0200
+++ gcc/testsuite/gcc.dg/pr109410.c	2023-04-12 13:42:27.249959585 +0200
@@ -0,0 +1,19 @@ 
+/* PR tree-optimization/109410 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+__attribute__((returns_twice)) int baz (int, int);
+
+int
+bar (int x)
+{
+  return x;
+}
+
+int
+foo (int x, int y)
+{
+  baz (x, y);
+  int a = bar (x);
+  return y || a == 42 || a > 42;
+}