On January 15, 2021 7:32:35 PM GMT+01:00, Jakub Jelinek <ja...@redhat.com>
wrote:
>Hi!
>
>On the following testcase, handle_builtin_memcmp in the strlen pass
>folds
>the memcmp into comparison of two MEM_REFs. But nothing triggers
>updating
>of addressable vars afterwards, so even when the parameters are no
>longer
>address taken, we force the parameters to stack and back anyway.
>
>The following patch fixes that.
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Hmm, we're currently doing it unconditionally at strategic points in the pass
pipeline. Is there no such point after the pass?
>2021-01-15 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/96271
> * tree-ssa-strlen.c (handle_builtin_memcmp): Add UPDATE_ADDRESS_TAKEN
> argument, set what it points to to true if optimizing memcmp into
> a simple MEM_REF comparison.
> (strlen_check_and_optimize_call): Add UPDATE_ADDRESS_TAKEN argument
> and pass it through to handle_builtin_memcmp.
> (check_and_optimize_stmt): Add UPDATE_ADDRESS_TAKEN argument
> and pass it through to strlen_check_and_optimize_call.
> (strlen_dom_walker): Replace m_cleanup_cfg with todo.
> (strlen_dom_walker::before_dom_children): Adjust for the above change,
> adjust check_and_optimize_stmt caller and or in into todo
> TODO_cleanup_cfg and/or TODO_update_address_taken.
> (printf_strlen_execute): Return todo instead of conditionally
> TODO_cleanup_cfg.
>
> * gcc.target/i386/pr96271.c: New test.
>
>--- gcc/tree-ssa-strlen.c.jj 2021-01-04 10:25:40.000000000 +0100
>+++ gcc/tree-ssa-strlen.c 2021-01-15 15:13:09.847839781 +0100
>@@ -3813,7 +3813,7 @@ use_in_zero_equality (tree res, bool exc
> return true when call is transformed, return false otherwise. */
>
> static bool
>-handle_builtin_memcmp (gimple_stmt_iterator *gsi)
>+handle_builtin_memcmp (gimple_stmt_iterator *gsi, bool
>*update_address_taken)
> {
> gcall *stmt = as_a <gcall *> (gsi_stmt (*gsi));
> tree res = gimple_call_lhs (stmt);
>@@ -3858,6 +3858,7 @@ handle_builtin_memcmp (gimple_stmt_itera
> boolean_type_node,
> arg1, arg2));
> gimplify_and_update_call_from_tree (gsi, res);
>+ *update_address_taken = true;
> return true;
> }
> }
>@@ -5110,7 +5111,7 @@ is_char_type (tree type)
>
> static bool
>strlen_check_and_optimize_call (gimple_stmt_iterator *gsi, bool
>*zero_write,
>- pointer_query &ptr_qry)
>+ bool *update_address_taken, pointer_query
>&ptr_qry)
> {
> gimple *stmt = gsi_stmt (*gsi);
>
>@@ -5179,7 +5180,7 @@ strlen_check_and_optimize_call (gimple_s
> return false;
> break;
> case BUILT_IN_MEMCMP:
>- if (handle_builtin_memcmp (gsi))
>+ if (handle_builtin_memcmp (gsi, update_address_taken))
> return false;
> break;
> case BUILT_IN_STRCMP:
>@@ -5341,12 +5342,13 @@ handle_integral_assign (gimple_stmt_iter
>/* Attempt to check for validity of the performed access a single
>statement
> at *GSI using string length knowledge, and to optimize it.
> If the given basic block needs clean-up of EH, CLEANUP_EH is set to
>- true. Return true to let the caller advance *GSI to the next
>statement
>- in the basic block and false otherwise. */
>+ true. If it is to update addressables at the end of the pass, set
>+ *UPDATE_ADDRESS_TAKEN to true. Return true to let the caller
>advance *GSI
>+ to the next statement in the basic block and false otherwise. */
>
> static bool
> check_and_optimize_stmt (gimple_stmt_iterator *gsi, bool *cleanup_eh,
>- pointer_query &ptr_qry)
>+ bool *update_address_taken, pointer_query &ptr_qry)
> {
> gimple *stmt = gsi_stmt (*gsi);
>
>@@ -5356,7 +5358,8 @@ check_and_optimize_stmt (gimple_stmt_ite
>
> if (is_gimple_call (stmt))
> {
>- if (!strlen_check_and_optimize_call (gsi, &zero_write, ptr_qry))
>+ if (!strlen_check_and_optimize_call (gsi, &zero_write,
>+ update_address_taken, ptr_qry))
> return false;
> }
> else if (!flag_optimize_strlen || !strlen_optimize)
>@@ -5488,7 +5491,7 @@ public:
> evrp (false),
> ptr_qry (&evrp, &var_cache),
> var_cache (),
>- m_cleanup_cfg (false)
>+ todo (0)
> { }
>
> virtual edge before_dom_children (basic_block);
>@@ -5503,9 +5506,8 @@ public:
> pointer_query ptr_qry;
> pointer_query::cache_type var_cache;
>
>- /* Flag that will trigger TODO_cleanup_cfg to be returned in strlen
>- execute function. */
>- bool m_cleanup_cfg;
>+ /* TODO_* flags for the pass. */
>+ int todo;
> };
>
> /* Callback for walk_dominator_tree. Attempt to optimize various
>@@ -5586,6 +5588,7 @@ strlen_dom_walker::before_dom_children (
> }
>
> bool cleanup_eh = false;
>+ bool update_address_taken = false;
>
> /* Attempt to optimize individual statements. */
> for (gimple_stmt_iterator gsi = gsi_start_bb (bb); !gsi_end_p (gsi); )
>@@ -5599,12 +5602,15 @@ strlen_dom_walker::before_dom_children (
> /* Reset search depth preformance counter. */
> ptr_qry.depth = 0;
>
>- if (check_and_optimize_stmt (&gsi, &cleanup_eh, ptr_qry))
>+ if (check_and_optimize_stmt (&gsi, &cleanup_eh,
>&update_address_taken,
>+ ptr_qry))
> gsi_next (&gsi);
> }
>
> if (cleanup_eh && gimple_purge_dead_eh_edges (bb))
>- m_cleanup_cfg = true;
>+ todo |= TODO_cleanup_cfg;
>+ if (update_address_taken)
>+ todo |= TODO_update_address_taken;
>
> bb->aux = stridx_to_strinfo;
> if (vec_safe_length (stridx_to_strinfo) && !strinfo_shared ())
>@@ -5715,7 +5721,7 @@ printf_strlen_execute (function *fun, bo
> loop_optimizer_finalize ();
> }
>
>- return walker.m_cleanup_cfg ? TODO_cleanup_cfg : 0;
>+ return walker.todo;
> }
>
> /* This file defines two passes: one for warnings that runs only when
>--- gcc/testsuite/gcc.target/i386/pr96271.c.jj 2021-01-15
>15:17:42.001780947 +0100
>+++ gcc/testsuite/gcc.target/i386/pr96271.c 2021-01-15
>15:17:25.447967002 +0100
>@@ -0,0 +1,11 @@
>+/* PR tree-optimization/96271 */
>+/* { dg-do compile } */
>+/* { dg-options "-O2 -mtune=intel -msse2 -masm=att" } */
>+/* { dg-final { scan-assembler "movq\t%xmm0, %r" { target { ! ia32 } }
>} } */
>+/* { dg-final { scan-assembler "movq\t%xmm1, %r" { target { ! ia32 } }
>} } */
>+
>+int
>+foo (double a, double b)
>+{
>+ return __builtin_memcmp (&a, &b, sizeof (double)) == 0;
>+}
>
> Jakub
