Re: [PATCH] tree-optimization/109849 - missed code hoisting

[Christophe Lyon <christophe.lyon@linaro.org>]

[-- Attachment #1: Type: text/plain, Size: 6660 bytes --]

Hi Richard,

On Tue, 23 May 2023 at 11:55, Richard Biener via Gcc-patches <
gcc-patches@gcc.gnu.org> wrote:

> The following fixes code hoisting to properly consider ANTIC_OUT instead
> of ANTIC_IN.  That's a bit expensive to re-compute but since we no
> longer iterate we're doing this only once per BB which should be
> acceptable.  This avoids missing hoistings to the end of blocks where
> something in the block clobbers the hoisted value.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, pushed.
>
>         PR tree-optimization/109849
>         * tree-ssa-pre.cc (do_hoist_insertion): Compute ANTIC_OUT
>         and use that to determine what to hoist.
>
>         * gcc.dg/tree-ssa/ssa-hoist-8.c: New testcase.
>

This patch causes a regression on aarch64:
gcc.target/aarch64/sve/fmla_2.c: \\tst1d found 3 times
 FAIL: gcc.target/aarch64/sve/fmla_2.c scan-assembler-times \\tst1d 2


We used to generate:
        mov     x6, 0
        mov     w7, 55
        whilelo p7.d, wzr, w7
        .p2align 3,,7
.L2:
        ld1d    z30.d, p7/z, [x5, x6, lsl 3]
        ld1d    z31.d, p7/z, [x4, x6, lsl 3]
        cmpne   p6.d, p7/z, z30.d, #0
        ld1d    z30.d, p7/z, [x3, x6, lsl 3]
        ld1d    z29.d, p6/z, [x2, x6, lsl 3]
        movprfx z28, z30
        fmla    z28.d, p6/m, z31.d, z29.d
        fmla    z31.d, p6/m, z30.d, z29.d
        st1d    z28.d, p7, [x1, x6, lsl 3]
        st1d    z31.d, p7, [x0, x6, lsl 3]
        incd    x6
        whilelo p7.d, w6, w7
        b.any   .L2


But now:
        mov     x6, 0
        mov     w7, 55
        ptrue   p4.b, all
        whilelo p7.d, wzr, w7
        .p2align 3,,7
.L2:
        ld1d    z30.d, p7/z, [x5, x6, lsl 3]
        ld1d    z31.d, p7/z, [x4, x6, lsl 3]
        cmpne   p6.d, p7/z, z30.d, #0
        cmpeq   p5.d, p7/z, z30.d, #0
        ld1d    z29.d, p6/z, [x2, x6, lsl 3]
        ld1d    z28.d, p6/z, [x3, x6, lsl 3]
        ld1d    z30.d, p5/z, [x3, x6, lsl 3]
        movprfx z27, z31
        fmla    z27.d, p4/m, z29.d, z28.d
        movprfx z30.d, p6/m, z28.d
        fmla    z30.d, p6/m, z31.d, z29.d
        st1d    z27.d, p6, [x0, x6, lsl 3]
        st1d    z30.d, p7, [x1, x6, lsl 3]
        st1d    z31.d, p5, [x0, x6, lsl 3]
        incd    x6
        whilelo p7.d, w6, w7
        b.any   .L2


Christophe

---
>  gcc/testsuite/gcc.dg/tree-ssa/ssa-hoist-8.c | 22 ++++++++++
>  gcc/tree-ssa-pre.cc                         | 48 ++++++++++++++++++---
>  2 files changed, 64 insertions(+), 6 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/ssa-hoist-8.c
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-hoist-8.c
> b/gcc/testsuite/gcc.dg/tree-ssa/ssa-hoist-8.c
> new file mode 100644
> index 00000000000..66bb48e0dc1
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-hoist-8.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-pre-stats" } */
> +
> +int mem;
> +void foo ();
> +int bar (int flag)
> +{
> +  int res;
> +  foo ();
> +  /* Hoist the load of mem here even though foo () clobbers it.  */
> +  if (flag)
> +    res = mem;
> +  else
> +    {
> +      res = mem;
> +      mem = 2;
> +    }
> +  return res;
> +}
> +
> +/* { dg-final { scan-tree-dump "HOIST inserted: 1" "pre" } } */
> +/* { dg-final { scan-tree-dump-times " = mem;" 1 "pre" } } */
> diff --git a/gcc/tree-ssa-pre.cc b/gcc/tree-ssa-pre.cc
> index 1f7eea93c16..d56431b4145 100644
> --- a/gcc/tree-ssa-pre.cc
> +++ b/gcc/tree-ssa-pre.cc
> @@ -3622,19 +3622,51 @@ do_hoist_insertion (basic_block block)
>        && stmt_ends_bb_p (gsi_stmt (last)))
>      return false;
>
> -  /* Compute the set of hoistable expressions from ANTIC_IN.  First
> compute
> +  /* We have multiple successors, compute ANTIC_OUT by taking the
> intersection
> +     of all of ANTIC_IN translating through PHI nodes.  Note we do not
> have to
> +     worry about iteration stability here so just intersect the
> expression sets
> +     as well.  This is a simplification of what we do in
> compute_antic_aux.  */
> +  bitmap_set_t ANTIC_OUT = bitmap_set_new ();
> +  bool first = true;
> +  FOR_EACH_EDGE (e, ei, block->succs)
> +    {
> +      if (first)
> +       {
> +         phi_translate_set (ANTIC_OUT, ANTIC_IN (e->dest), e);
> +         first = false;
> +       }
> +      else if (!gimple_seq_empty_p (phi_nodes (e->dest)))
> +       {
> +         bitmap_set_t tmp = bitmap_set_new ();
> +         phi_translate_set (tmp, ANTIC_IN (e->dest), e);
> +         bitmap_and_into (&ANTIC_OUT->values, &tmp->values);
> +         bitmap_and_into (&ANTIC_OUT->expressions, &tmp->expressions);
> +         bitmap_set_free (tmp);
> +       }
> +      else
> +       {
> +         bitmap_and_into (&ANTIC_OUT->values, &ANTIC_IN
> (e->dest)->values);
> +         bitmap_and_into (&ANTIC_OUT->expressions,
> +                          &ANTIC_IN (e->dest)->expressions);
> +       }
> +    }
> +
> +  /* Compute the set of hoistable expressions from ANTIC_OUT.  First
> compute
>       hoistable values.  */
>    bitmap_set hoistable_set;
>
> -  /* A hoistable value must be in ANTIC_IN(block)
> +  /* A hoistable value must be in ANTIC_OUT(block)
>       but not in AVAIL_OUT(BLOCK).  */
>    bitmap_initialize (&hoistable_set.values, &grand_bitmap_obstack);
>    bitmap_and_compl (&hoistable_set.values,
> -                   &ANTIC_IN (block)->values, &AVAIL_OUT (block)->values);
> +                   &ANTIC_OUT->values, &AVAIL_OUT (block)->values);
>
>    /* Short-cut for a common case: hoistable_set is empty.  */
>    if (bitmap_empty_p (&hoistable_set.values))
> -    return false;
> +    {
> +      bitmap_set_free (ANTIC_OUT);
> +      return false;
> +    }
>
>    /* Compute which of the hoistable values is in AVAIL_OUT of
>       at least one of the successors of BLOCK.  */
> @@ -3652,11 +3684,14 @@ do_hoist_insertion (basic_block block)
>
>    /* Short-cut for a common case: availout_in_some is empty.  */
>    if (bitmap_empty_p (&availout_in_some))
> -    return false;
> +    {
> +      bitmap_set_free (ANTIC_OUT);
> +      return false;
> +    }
>
>    /* Hack hoitable_set in-place so we can use
> sorted_array_from_bitmap_set.  */
>    bitmap_move (&hoistable_set.values, &availout_in_some);
> -  hoistable_set.expressions = ANTIC_IN (block)->expressions;
> +  hoistable_set.expressions = ANTIC_OUT->expressions;
>
>    /* Now finally construct the topological-ordered expression set.  */
>    vec<pre_expr> exprs = sorted_array_from_bitmap_set (&hoistable_set);
> @@ -3731,6 +3766,7 @@ do_hoist_insertion (basic_block block)
>      }
>
>    exprs.release ();
> +  bitmap_set_free (ANTIC_OUT);
>
>    return new_stuff;
>  }
> --
> 2.35.3
>